9608 trunk #236

jasobrown · 2018-06-16T22:10:23Z

No description provided.

jasobrown · 2018-06-16T22:17:00Z

build.xml

+    <property name="ecj.version" value="4.6.1"/>
+    <property name="ohc.version" value="0.5.1"/>
+
+    <!--


nit: let's move this java version section above the dependencies section

jasobrown · 2018-06-16T22:23:00Z

build.xml

@@ -462,7 +501,9 @@
          <dependency groupId="com.googlecode.concurrent-trees" artifactId="concurrent-trees" version="2.4.0" />
          <dependency groupId="com.github.ben-manes.caffeine" artifactId="caffeine" version="2.3.5" />
          <dependency groupId="org.jctools" artifactId="jctools-core" version="1.2.1"/>
-          <dependency groupId="org.ow2.asm" artifactId="asm" version="5.0.4" />
+          <dependency groupId="org.ow2.asm" artifactId="asm" version="6.2" />


nit: maybe introduce a property with the version for org.ow2.asm, like what you've done for ecj.version and others?

jasobrown · 2018-06-16T22:24:21Z

build.xml

@@ -703,6 +744,13 @@
        </patternset>
        <mapper type="flatten"/>
      </unzip>
+
+      <delete>


why do you need to delete the netty and asm jars here?

Otherwise an older asm (and previously an older netty version as well) landed in build/lib/jars via a transitive dependency.

jasobrown · 2018-06-29T19:02:50Z

conf/jvm11.options

+# Java 11 (and newer) GC logging options:
+# See description of https://bugs.openjdk.java.net/browse/JDK-8046148 for details about the syntax
+# The following is the equivalent to -XX:+PrintGCDetails -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=10 -XX:GCLogFileSize=10M
+#-Xlog:gc=info,heap*=trace,age*=debug,safepoint=info,promotion*=trace:file=/var/log/cassandra/gc.log:time,uptime,pid,tid,level:filecount=10,filesize=10240


minor nit: filesize=10240 is the file size in bytes. change to 10485760 if you actually want 10MB files.

nice catch!

jasobrown · 2018-06-29T19:03:00Z

conf/cassandra-env.sh

+    if ! grep -q "^-[X]log:gc" $CASSANDRA_CONF/jvm.options ; then # [X] to prevent ccm from replacing this line
+        # only add -Xlog:gc if it's not mentioned in jvm.options file
+        mkdir -p ${CASSANDRA_HOME}/logs
+        JVM_OPTS="$JVM_OPTS -Xlog:gc=info,heap*=trace,age*=debug,safepoint=info,promotion*=trace:file=${CASSANDRA_HOME}/logs/gc.log:time,uptime,pid,tid,level:filecount=10,filesize=10240"


minor nit: filesize=10240 is the file size in bytes. change to 10485760 if you actually want 10MB files.

nice catch!

jasobrown · 2018-06-29T19:25:52Z

src/java/org/apache/cassandra/io/util/FileUtils.java

        }
        isCleanerAvailable = canClean;
    }

+    public static boolean isDirectBuffer(ByteBuffer buf)
+    {
+        return clsDirectBuffer.isInstance(buf);


Can't we just call ByteBuffer.isDirect() instead?

jasobrown · 2018-06-29T19:31:31Z

src/java/org/apache/cassandra/security/ThreadAwareSecurityManager.java

@@ -208,7 +225,5 @@ public void checkPackageAccess(String pkg)
            RuntimePermission perm = new RuntimePermission("accessClassInPackage." + pkg);
            throw new AccessControlException("access denied: " + perm, perm);
        }
-
-        super.checkPackageAccess(pkg);


Why don't we need to call super.checkPackageAccess() anymore?

Otherwise JavaScript based UDFs stop working due to JPMS, that would raise errors like
Caused by: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "accessClassInPackage.jdk.internal.org.objectweb.asm")

jasobrown · 2018-06-29T19:33:56Z

src/java/org/apache/cassandra/utils/NativeLibrary.java

@@ -78,7 +77,14 @@
    static
    {
        FILE_DESCRIPTOR_FD_FIELD = FBUtilities.getProtectedField(FileDescriptor.class, "fd");
-        FILE_CHANNEL_FD_FIELD = FBUtilities.getProtectedField(FileChannelImpl.class, "fd");
+        try


why is this in a try/catch block now?

Because of the Class.forName declaring the checked exception.

ahh, ok missed that one

jasobrown · 2018-06-29T20:01:21Z

src/java/org/apache/cassandra/cql3/functions/JavaBasedUDFunction.java

+                    {
+                        // For Java 11 try to see whether the class actually exists and read the
+                        // class file data via the class' module. (This is necessary at least
+                        // for 9-ea build 123)


Is this still true as of java11 (ea 19)?

Hm - seems that was actually a bug in Java and it's been fixed.

jasobrown · 2018-06-30T19:30:28Z

src/java/org/apache/cassandra/io/util/FileUtils.java

+    private static Class clsDirectBuffer;
+    private static MethodHandle mhDirectBufferCleaner;
+    private static MethodHandle mhCleanerClean;
+    private static MethodHandle mhDirectBufferAddress;


mhDirectBufferAddress isn't used anywhere. can we eliminate it?

jasobrown · 2018-06-30T19:38:24Z

src/java/org/apache/cassandra/io/util/FileUtils.java

            canClean = true;
        }
        catch (Throwable t)
        {
+            logger.info("Cannot initialize un-mmaper. Compacted data files will not be removed promptly.", t);


The original log message here was rather awful. I'm not sure sure what actions an operator could take after seeing this, and Compacted data files will not be removed promptly is sufficiently vague.

Maybe we could say something like: `Cannot initialize optimized memory deallocator. Some data, both in-memory and on-disk, may live longer due to garbage collection."

Sounds good

jasobrown · 2018-06-30T19:42:38Z

src/java/org/apache/cassandra/io/util/FileUtils.java

+        cleanerClean(buf, false);
+    }
+
+    public static void cleanerClean(ByteBuffer buf, boolean cleanAttachment)


I don't see any code path where cleanAttachment can be true. Am I missing something?

yikes - that was wrong. good catch!

jasobrown · 2018-06-30T20:01:06Z

src/java/org/apache/cassandra/io/util/FileUtils.java

    public static File createTempFile(String prefix, String suffix, File directory)
    {
        try
        {
-            return File.createTempFile(prefix, suffix, directory);
+            // Do not use java.io.File.createTempFile(), because some tests rely on the


Do the tests that require monotonicity in the file version numbers also require the versions to be related to wall-clock time? If not, and the monotonicity / uniqueness guarantees are sufficient, you can simplify the while loop below to get just the next value by tempFileNum.getAndIncrement()

Right, I think that's reasonable.

patch by Robert Stupp; reviewed by Jason Brown and Eduard Tudenhöfner for CASSANDRA-9608

use chronicle-core snapshot build to run on Java 11 the snapshot-build of chronicle-core solves the "junit.framework.AssertionFailedError: java.lang.NoSuchFieldException: reservedMemory" issue at net.openhft.chronicle.core.Jvm.<clinit>(Jvm.java:87) see OpenHFT/Chronicle-Core#68

jasobrown · 2018-07-21T13:49:53Z

conf/jvm11-clients.options

+###########################################################################
+#                         jvm11-clients.options                           #
+#                                                                         #
+# See jvm-clients.options. This file is specific for Java 8 and newer.    #


nit: this should say for Java 11 and newer

jasobrown · 2018-07-23T10:38:59Z

build.xml

-    <property name="source.version" value="1.8"/>
-    <property name="target.version" value="1.8"/>
+    <!--
+    We specific '8' instead of '1.8' in source.version to indicate that that this build requires Java 11 _and_ Java 8.


petty nit: We specify

jasobrown · 2018-07-23T11:11:06Z

build.xml

@@ -1665,7 +1763,7 @@
    <echo message="Mem size : ${mem.size}"/>
  </target>

-  <target name="test" depends="build-test,get-cores,get-mem,stress-build" description="Parallel Test Runner">
+  <target name="test" depends="build-test" description="Parallel Test Runner">


Why does this not depend on get-cores,get-mem anymore? Otherwise, those targets are unused in the build.xml

Oops, re-added

jasobrown · 2018-07-23T11:20:20Z

src/java/org/apache/cassandra/cql3/functions/JavaBasedUDFunction.java

@@ -591,14 +592,17 @@ private NameEnvironmentAnswer findType(String className)

            String resourceName = className.replace('.', '/') + ".class";

-            try (InputStream is = UDFunction.udfClassLoader.getResourceAsStream(resourceName))
+            try


Not sure why we need a nested try block here.

Ah - I've changed that part twice. But yea, one try-catch is absolutely sufficient.

jasobrown · 2018-07-23T11:34:16Z

src/java/org/apache/cassandra/io/util/FileUtils.java

            JVMStabilityInspector.inspectThrowable(t);
-            logger.info("Cannot initialize un-mmaper.  (Are you using a non-Oracle JVM?)  Compacted data files will not be removed promptly.  Consider using an Oracle JVM or using standard disk access mode");
+            throw new RuntimeException(t);


So, this is a change in semantics (throwing a RuntimeException). Previously, we would just complain if we couldn't get to the buffer cleaner, now we're basically terminating the process. Is that intended? If it is, at minimum the log statement a few lines up should be FATAL.

Pretty much every JVM should have a Cleaner nowadays. Not having would break a lot of assumṕtions IMO.
Changed the log message to ERROR.

jasobrown · 2018-07-23T11:37:03Z

src/java/org/apache/cassandra/io/util/FileUtils.java

+
+                // We have a positive long here, which is safe to use for example
+                // for CommitLogTest.
+                String timePart = Long.toString(num);


rename this variable, or maybe just move Long.toString(num) into the next line.

jasobrown · 2018-07-23T11:38:57Z

src/java/org/apache/cassandra/io/util/FileUtils.java

-            DirectBuffer db = (DirectBuffer) buffer;
-            if (db.cleaner() != null)
-                db.cleaner().clean();
+            if (buffer.isDirect())


This check is redundant as you've already performed the !buffer.isDirect() earlier in this method (and bailed if it isn't)

jasobrown · 2018-07-23T11:44:44Z

src/java/org/apache/cassandra/io/util/FileUtils.java

-                db.cleaner().clean();
+            if (buffer.isDirect())
+            {
+                Object cleaner = mhDirectBufferCleaner.bindTo(buffer).invoke();


Can you add some comments here? It's not clear what's going on, but more importantly, why there's two code paths.

Woops. There shouldn't be any else.

jasobrown · 2018-07-23T11:49:48Z

src/java/org/apache/cassandra/utils/NativeLibrary.java

@@ -78,7 +77,14 @@
    static
    {
        FILE_DESCRIPTOR_FD_FIELD = FBUtilities.getProtectedField(FileDescriptor.class, "fd");
-        FILE_CHANNEL_FD_FIELD = FBUtilities.getProtectedField(FileChannelImpl.class, "fd");
+        try


ahh, ok missed that one

jasobrown · 2018-07-26T13:09:14Z

Is javaexec.in.sh needed anymore? Looks like all the java checks are in cassandra.in.sh now. The file is not referenced by dtests or ccm, either.

snazy · 2018-07-26T14:31:15Z

No, it's no longer needed. I removed it.

jasobrown · 2018-07-26T17:27:10Z

Committed as sha 6ba2fb9395226491872b41312d978a169f36fcdb

(cherry picked from commit 5f0479d)

jasobrown commented Jun 29, 2018

View reviewed changes

jasobrown commented Jun 30, 2018

View reviewed changes

snazy force-pushed the 9608-trunk branch from b46f4b3 to 77c26d1 Compare July 2, 2018 12:01

snazy force-pushed the 9608-trunk branch 2 times, most recently from b63587c to 014b837 Compare July 20, 2018 11:39

snazy added 8 commits July 23, 2018 10:41

make C* compile and run on Java 11 and Java 8

f256c15

patch by Robert Stupp; reviewed by Jason Brown and Eduard Tudenhöfner for CASSANDRA-9608

Update asm to 6.2

e00e8f9

address ecj resource-warnings

3f914ce

enable dtests

ea71915

address review comments

d85a8be

another resource-warning

a254fdf

Eliminate duplicate method, assume that cleaner is always available

d6b8192

snazy force-pushed the 9608-trunk branch from 014b837 to d6b8192 Compare July 23, 2018 08:42

typo

7a6a911

jasobrown commented Jul 23, 2018

View reviewed changes

snazy added 2 commits July 23, 2018 14:30

address review comments

3924ae8

PROPOSAL for the lock-spin-loop !!!

6dc75d8

snazy force-pushed the 9608-trunk branch from 6c792d3 to 6dc75d8 Compare July 23, 2018 12:31

migrate to ReentrantLock

affa0b9

remove superfluous file

47b645d

jasobrown closed this Jul 26, 2018

snazy deleted the 9608-trunk branch May 4, 2020 09:39

blambov pushed a commit to blambov/cassandra that referenced this pull request Oct 1, 2021

STAR-843: Use ULID based sstable UIDs (cassandra/ds-trunk) (apache#236)

5f0479d

jacek-lewandowski added a commit to jacek-lewandowski/cassandra that referenced this pull request Mar 7, 2022

STAR-843: Use ULID based sstable UIDs (cassandra/ds-trunk) (apache#236)

6bdddd7

(cherry picked from commit 5f0479d)

blambov pushed a commit to blambov/cassandra that referenced this pull request Mar 21, 2022

STAR-843: Use ULID based sstable UIDs (cassandra/ds-trunk) (apache#236)

e0b67f5

(cherry picked from commit 5f0479d)

9608 trunk #236

9608 trunk #236

Conversation

jasobrown commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasobrown commented Jul 26, 2018

snazy commented Jul 26, 2018

jasobrown commented Jul 26, 2018