You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org> on 2023/06/16 15:14:28 UTC
[GitHub] [cassandra] ekaterinadimitrova2 opened a new pull request, #2421: Cassandra 18180 trunk
ekaterinadimitrova2 opened a new pull request, #2421:
URL: https://github.com/apache/cassandra/pull/2421
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #2421: Cassandra 18180 trunk
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #2421:
URL: https://github.com/apache/cassandra/pull/2421#discussion_r1232772101
##########
test/anttasks/org/apache/cassandra/anttasks/TestNameCheckTask.java:
##########
@@ -45,6 +45,11 @@ public TestNameCheckTask()
{
}
+ public static void main(String[] args)
+ {
+ new TestNameCheckTask().execute();
Review Comment:
This is incorrect, as we no longer fail the ant task.
It completes successfully if I test with an invalid test name; there is only one exception in the logs, which people will miss
##########
src/java/org/apache/cassandra/utils/concurrent/Ref.java:
##########
@@ -91,7 +93,7 @@
* Once the Ref.GlobalState has been completely released, the Tidy method is called and it removes the global reference
* to itself so it may also be collected.
*/
-public final class Ref<T> implements RefCounted<T>
+public class Ref<T> implements RefCounted<T>
Review Comment:
I am worried about having this public class not final; I think I mentioned it already in one of the commits yesterday, but adding here for completeness.
##########
test/anttasks/org/apache/cassandra/anttasks/TestNameCheckTask.java:
##########
@@ -45,6 +45,11 @@ public TestNameCheckTask()
{
}
+ public static void main(String[] args)
+ {
+ new TestNameCheckTask().execute();
Review Comment:
I did some testing, and the issue with the ant task is only with 11. So as a workaround, we can make the task to run at the moment only with JDK11.
While it is good to run it with both JDKs, the output is the same with both JDKs.
So I suggest we open a follow-up ticket and make it run only with JDK11 here to unblock ourselves.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 closed pull request #2421: Cassandra 18180 trunk
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 closed pull request #2421: Cassandra 18180 trunk
URL: https://github.com/apache/cassandra/pull/2421
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on pull request #2421: Cassandra 18180 trunk
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #2421: Cassandra 18180 trunk
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #2421:
URL: https://github.com/apache/cassandra/pull/2421#discussion_r1237840643
##########
build.xml:
##########
@@ -1853,7 +1866,10 @@
<echo file=".idea/compiler.xml"><![CDATA[<?xml version="1.0" encoding="UTF-8"?>
<project version="4">
<component name="JavacSettings">
- <option name="ADDITIONAL_OPTIONS_STRING" value="--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED" />
+ <option name="ADDITIONAL_OPTIONS_STRING" value="--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED
+ --add-exports java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED
+ --add-exports java.base/jdk.internal.ref=ALL-UNNAMED
+ --add-exports java.base/sun.nio.ch=ALL-UNNAMED" />
Review Comment:
It seems John Meredith added two weeks ago JDK17 target for IntelliJ :D
We need to add these exports there too.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #2421: Cassandra 18180 trunk
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #2421:
URL: https://github.com/apache/cassandra/pull/2421#discussion_r1238918758
##########
build.xml:
##########
@@ -1853,7 +1866,10 @@
<echo file=".idea/compiler.xml"><![CDATA[<?xml version="1.0" encoding="UTF-8"?>
<project version="4">
<component name="JavacSettings">
- <option name="ADDITIONAL_OPTIONS_STRING" value="--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED" />
+ <option name="ADDITIONAL_OPTIONS_STRING" value="--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED
+ --add-exports java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED
+ --add-exports java.base/jdk.internal.ref=ALL-UNNAMED
+ --add-exports java.base/sun.nio.ch=ALL-UNNAMED" />
Review Comment:
@djatnieks I just pushed commit to fix this. Tested locally
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] djatnieks commented on a diff in pull request #2421: Cassandra 18180 trunk
Posted by "djatnieks (via GitHub)" <gi...@apache.org>.
djatnieks commented on code in PR #2421:
URL: https://github.com/apache/cassandra/pull/2421#discussion_r1235988404
##########
src/java/org/apache/cassandra/utils/concurrent/Ref.java:
##########
@@ -91,7 +93,7 @@
* Once the Ref.GlobalState has been completely released, the Tidy method is called and it removes the global reference
* to itself so it may also be collected.
*/
-public final class Ref<T> implements RefCounted<T>
+public class Ref<T> implements RefCounted<T>
Review Comment:
Okay, I could change `DirectBufferRef` to wrap a `Ref` instead of extending `Ref` - then `Ref` can remain `final`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] djatnieks commented on a diff in pull request #2421: Cassandra 18180 trunk
Posted by "djatnieks (via GitHub)" <gi...@apache.org>.
djatnieks commented on code in PR #2421:
URL: https://github.com/apache/cassandra/pull/2421#discussion_r1236195001
##########
test/anttasks/org/apache/cassandra/anttasks/TestNameCheckTask.java:
##########
@@ -45,6 +45,11 @@ public TestNameCheckTask()
{
}
+ public static void main(String[] args)
+ {
+ new TestNameCheckTask().execute();
Review Comment:
Thank you! I made a test class with an invalid name and see that while there is an error logged, the build continues.
I think I found another option - adding `failonerror="yes"` to the java task, like this:
```
<java taskname="check-test-names" fork="true" failonerror="yes" classname="org.apache.cassandra.anttasks.TestNameCheckTask" classpath="${test.classes}">
```
With this change, running `ant build-test` with an invalid test name both logs an error and stops the build with an error.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on pull request #2421: Cassandra 18180 trunk
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on PR #2421:
URL: https://github.com/apache/cassandra/pull/2421#issuecomment-1677635484
Committed
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org