ekaterinadimitrova2 commented on code in PR #2226:
URL: https://github.com/apache/cassandra/pull/2226#discussion_r1139500747
##########
.build/cassandra-deps-template.xml:
##########
@@ -211,6 +213,40 @@
<groupId>net.openhft</groupId>
<artifactId>chronicle-threads</artifactId>
</dependency>
+ <dependency>
+ <!-- transitive to chronicle-core -->
Review Comment:
So, in order to get a higher version, we add them explicitly. Can we put a
short explanation as saying "transitive", the reason for adding them explicitly
doesn't come immediately to my mind? It reads a bit confusing
##########
src/java/org/apache/cassandra/utils/binlog/BinLog.java:
##########
@@ -75,6 +77,12 @@ public class BinLog implements Runnable
public static final String VERSION = "version";
public static final String TYPE = "type";
+ static
+ {
+ //
https://github.com/OpenHFT/Chronicle-Core/blob/chronicle-core-2.23.36/src/main/java/net/openhft/chronicle/core/announcer/Announcer.java#L32-L33
+ System.setProperty("chronicle.announcer.disable", "true");
Review Comment:
Link to the discussion from the commit so we do not lose it -
https://github.com/apache/cassandra/commit/30e8efb97475e3d53e9f5261ec9a488b1bef1980#r104781187
##########
.build/parent-pom-template.xml:
##########
@@ -753,21 +804,13 @@
<dependency>
<groupId>net.openhft</groupId>
<artifactId>chronicle-threads</artifactId>
- <version>2.20.111</version>
- <exclusions>
- <exclusion>
- <artifactId>affinity</artifactId>
- <groupId>net.openhft</groupId>
- </exclusion>
- <exclusion>
- <artifactId>jna</artifactId>
- <groupId>net.java.dev.jna</groupId>
- </exclusion>
- <exclusion>
- <artifactId>jna-platform</artifactId>
- <groupId>net.java.dev.jna</groupId>
- </exclusion>
- </exclusions>
+ <version>2.23.25</version>
+ </dependency>
+ <dependency>
+ <!-- transitive to chronicle-queue -->
+ <groupId>net.openhft</groupId>
+ <artifactId>posix</artifactId>
+ <version>2.24ea4</version>
Review Comment:
ea version? Shouldn't we use 2.23.2?
##########
conf/jvm11-server.options:
##########
@@ -61,6 +61,7 @@
-Djdk.attach.allowAttachSelf=true
--add-exports java.base/jdk.internal.misc=ALL-UNNAMED
--add-exports java.base/jdk.internal.ref=ALL-UNNAMED
+--add-exports java.base/jdk.internal.util=ALL-UNNAMED
Review Comment:
```
the 8x faster version of BytesInternal.contentEqual*(..) (and
BytesStore.equals(..)) on jdk11+ (using SIMD comparison instruction) needs
`--illegal-access=permit --add-exports java.base/jdk.internal.ref=ALL-UNNAMED
--add-exports java.base/jdk.internal.util=ALL-UNNAMED`, ref
[here](https://github.com/OpenHFT/Chronicle-Bytes/compare/chronicle-bytes-2.20.111...chronicle-bytes-2.23.33#diff-7be2a8746677c85a85b22da547a6132e51ba0a5b61538f9b19d3e8f6da43be20R115)
this opens for
[CASSANDRA-13903](https://issues.apache.org/jira/browse/CASSANDRA-13903)
```
Currently we do not use `BytesInternal.contentEqual*(..) (and
BytesStore.equals(..)) ` but moving to a new version we need those exports for
those in order to use them. Is that correct statement? Also, the reference
points to a long list of commits not sure whether that was the right link you
wanted to post
##########
.build/parent-pom-template.xml:
##########
@@ -705,7 +756,7 @@
<dependency>
<groupId>net.openhft</groupId>
<artifactId>chronicle-queue</artifactId>
- <version>5.20.123</version>
Review Comment:
Which bom are those? It would be nice to add it here in a comment
##########
src/java/org/apache/cassandra/utils/binlog/BinLog.java:
##########
@@ -75,6 +77,12 @@ public class BinLog implements Runnable
public static final String VERSION = "version";
public static final String TYPE = "type";
+ static
+ {
+ //
https://github.com/OpenHFT/Chronicle-Core/blob/chronicle-core-2.23.36/src/main/java/net/openhft/chronicle/core/announcer/Announcer.java#L32-L33
+ System.setProperty("chronicle.announcer.disable", "true");
Review Comment:
Considering your comment, I would say we can add a comment that enabling
this will lead only to duplication of information and logs pollution
##########
src/java/org/apache/cassandra/utils/binlog/BinLog.java:
##########
@@ -170,6 +178,7 @@ public synchronized void stop() throws InterruptedException
shouldContinue = false;
sampleQueue.put(NO_OP);
+ BackgroundResourceReleaser.stop();
Review Comment:
To confirm my understanding - `BackgroundResourceReleaser.stop();` was
missed to be done in the past? Bug?
--
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]
