Repository navigation
Adding staleness concept to JDBC_PING - #47971
Conversation
b8086a8 to
abaae9a
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.model.singleUseObject.SingleUseObjectModelTest#testClusterKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNodeKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.AuthenticationSessionTest#testConcurrentAuthenticationSessionsCreationKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.UserSessionProviderModelTest#testCreateUserSessionsParallelKeycloak CI - Store Model Tests |
Closes keycloak#47970 Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
abaae9a to
ff49dbd
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.model.singleUseObject.SingleUseObjectModelTest#testClusterKeycloak CI - Store Model Tests org.keycloak.testsuite.model.singleUseObject.SingleUseObjectModelTest#testClusterKeycloak CI - Store Model Tests |
|
@pruivo - can you please review this PR when you have the time? Thanks! |
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
|
@ryanemerson - I've now added a test, and refined the logic a bit more. Please review when you have the time. Thanks! |
| "coordinated_by varchar(200), " + | ||
| "PRIMARY KEY (address) )"; | ||
| insert_single_sql="INSERT INTO jgroups values (?, ?, ?, ?, ?, ?, ?)"; | ||
| select_all_pingdata_sql="SELECT address, name, ip, coord, coordinated_by, last_update FROM jgroups WHERE cluster=?"; |
There was a problem hiding this comment.
It seems this is only required by JdbcPing2Test, as we're initializing these values in the JGroupsConfigurator, therefore it would be better if we added this to the KEYCLOAK_JDBC_PING2_FOR_TESTING constructor instead of in the production code.
There was a problem hiding this comment.
Once we move this to upstream, those SQLs need to be updated there as well. That's why I want to keep them here.
There was a problem hiding this comment.
Hmm I would suggest we create the upstream PR before we forget this detail, it doesn't feel right to include this code in src/main just for the purpose of a reminder. As the class variables in JDBC_PING2 are not initialized in a constructor, it is done inline, we won't be able to copy and paste this code in its current form directly anyway.
There was a problem hiding this comment.
Having the table name hardcoded here doesn't seem right. Do we have 2 tables now? jgroups and jgroups_ping?
There was a problem hiding this comment.
@ryanemerson - I've moved it from a constructor to a an initializer, as this might make it more closely aligned with what's happening in the parent class. Let me know your thoughts.
About upstream: Not sure if this will land in upstream as an update to JDBC_PING2, or rather a new variant JDBC_PING3 with some more opinionated defaults, and some pruned functionality for better maintenance.
To validate this approach, I'd like to start in Keycloak's repository.
There was a problem hiding this comment.
@pruivo - The name jgroups is the one used JGROUPS_PING2, and as I was about to move it upstream eventually, I was aligning with that name (and it is hardcoded there as well).
The person who implemented it for Keycloak chose the name jgroups_table, and that's how we ended up with a different name.
There was a problem hiding this comment.
I've moved it from a constructor to a an initializer
-1 from me. Configure the new SQL queries in model/infinispan/src/test/resources/jdbc-h2.xml.
Not sure if this will land in upstream as an update to JDBC_PING2
This is a backwards-incompatible change. Remember, there are JGroups users outside Infinispan and Keycloak, and they most likely don't have Liquibase or any tool to update the table schema :)
There was a problem hiding this comment.
The person who implemented it for Keycloak chose the name jgroups_table
Not entirely true, JDBC_PING has JGROUPSPING for table name: https://github.com/belaban/JGroups/blob/master/src/org/jgroups/protocols/JDBC_PING.java#L57
There was a problem hiding this comment.
-1 from me. Configure the new SQL queries in
model/infinispan/src/test/resources/jdbc-h2.xml.
+1 to adding here, I wasn't aware of this file before.
This is a backwards-incompatible change. Remember, there are JGroups users outside Infinispan and Keycloak, and they most likely don't have Liquibase or any tool to update the table schema :)
That's a good point, at this stage JDBC_PING3 is a more realistic upstream contribution.
There was a problem hiding this comment.
Not entirely true, JDBC_PING has JGROUPSPING for table name:
I've been looking at JDBC_PING2 which is using jgroups, see https://github.com/belaban/JGroups/blob/master/src/org/jgroups/protocols/JDBC_PING2.java#L56.
I've added it to the jdbc-h2.xml file as you both suggested.
| s.setString(3, clusterName); // cluster name | ||
| s.setString(4, "127.0.0.1:0"); // ip = new IpAddress("localhost", 0).toString() | ||
| s.setBoolean(5, false); // coord | ||
| s.setLong(6, TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis())); // coord |
There was a problem hiding this comment.
Should this be Time.currentTimeMillis() instead of System.currentTimeMillis()?
Comment needs updating to reflect that it's the staleness timestamp "last_update".
|
@ryanemerson - this should now be ready for a re-review. Please have a look when you have the time. Thanks! |
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
238d5ac to
e25f3c1
Compare
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
This improves Keylcoak's behavior in two ways:
Not covered here:
Closes #47970