ekaterinadimitrova2 commented on PR #2421: URL: https://github.com/apache/cassandra/pull/2421#issuecomment-1595364906
I still have to dig into the GaloisCaounterMode but I wanted to give you first review comments: 1) tested checkstyle and it seems it does not work as expected anymore. I suggested a workaround in comments 2)I am not sure it is a good idea to make Ref not final anymore 3)Did some archeology and talked to David Capwell about: `However, then I found that SyncUtil.force uses MemoryUtil.getAttachment and expects that the attachment may be Runnable. I'm not sure yet how that gets set though - I didn't any other setAttachment calls with Runnable except in a unit test. ` That check for instance of Runnable was added in CASSANDRA-18485. There is a PR attached that led me to this comment https://github.com/apache/cassandra/pull/2299#discussion_r1179737237 So I confirmed with David that this is used only for tests or in his own words: "this is only done for the ListenableFileSystem which needs to override sync so anything using CQLTester.InMemory should be hitting this code path so if those tests are passing... then prob fine" It seems he added comment in the PR and forgot to move it to the code itself. We agreed we can add something as part of this ticket so people do not have to do archeology again :-) 4) I also ran CI and I did not find any new failures https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/2378/workflows/cfb2f852-1a68-4a7f-8b61-c49efc5403f6 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
