[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea

["lidavidm (via GitHub)" <gi...@apache.org>]

lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1228007401


##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -53,9 +53,10 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator {
     if (propValue != null) {
       DEBUG = Boolean.parseBoolean(propValue);
     } else {
-      DEBUG = AssertionUtil.isAssertionsEnabled();
+      DEBUG = false;
     }
-    logger.info("Debug mode " + (DEBUG ? "enabled." : "disabled."));
+    logger.info("Debug mode " + (DEBUG ? "enabled."
+        : "disabled. Enable that by the following VM option -Darrow.memory.debug.allocator=true."));

Review Comment:
   ```suggestion
           : "disabled. Enable with the VM option -Darrow.memory.debug.allocator=true."));
   ```



##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,44 @@ public void testSetBytesBigEndian() {
     assertFalse(data.hasArray());
     assertFalse(data.isDirect());
     assertEquals(ByteOrder.BIG_ENDIAN, data.order());
-    try (ArrowBuf buf = allocator.buffer(expected.length)) {
+    try (BufferAllocator allocator = new RootAllocator(128);
+        ArrowBuf buf = allocator.buffer(expected.length)) {
       buf.setBytes(0, data);
       byte[] actual = new byte[expected.length];
       buf.getBytes(0, actual);
       assertArrayEquals(expected, actual);
     }
   }
 
+  @Test
+  public void testEnabledAssertion() {
+    ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+    ClassLoader.getSystemClassLoader().setDefaultAssertionStatus(true);
+    try (BufferAllocator allocator = new RootAllocator(128)) {
+      ArrowBuf buf = allocator.buffer(2);
+    } catch (Exception e) {
+      assertFalse(e.getMessage().contains("event log for:"));
+    }
+  }
+
+  @Test
+  public void testEnabledHistoricalLog() {
+    try {
+      ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+      Field fieldDebug = BaseAllocator.class.getField("DEBUG");
+      fieldDebug.setAccessible(true);
+      Field modifiersDebug = Field.class.getDeclaredField("modifiers");
+      modifiersDebug.setAccessible(true);
+      modifiersDebug.setInt(fieldDebug, fieldDebug.getModifiers() & ~Modifier.FINAL);
+      fieldDebug.set(null, true);
+      try (BufferAllocator allocator = new RootAllocator(128)) {
+        ArrowBuf buf = allocator.buffer(2);
+      } catch (Exception e) {
+        assertTrue(e.getMessage().contains("event log for:")); // JDK8, JDK11
+        fieldDebug.set(null, false);

Review Comment:
   put this in a `finally` block?



##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,44 @@ public void testSetBytesBigEndian() {
     assertFalse(data.hasArray());
     assertFalse(data.isDirect());
     assertEquals(ByteOrder.BIG_ENDIAN, data.order());
-    try (ArrowBuf buf = allocator.buffer(expected.length)) {
+    try (BufferAllocator allocator = new RootAllocator(128);
+        ArrowBuf buf = allocator.buffer(expected.length)) {
       buf.setBytes(0, data);
       byte[] actual = new byte[expected.length];
       buf.getBytes(0, actual);
       assertArrayEquals(expected, actual);
     }
   }
 
+  @Test
+  public void testEnabledAssertion() {
+    ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);

Review Comment:
   We need to reset the level after the test runs.



##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,44 @@ public void testSetBytesBigEndian() {
     assertFalse(data.hasArray());
     assertFalse(data.isDirect());
     assertEquals(ByteOrder.BIG_ENDIAN, data.order());
-    try (ArrowBuf buf = allocator.buffer(expected.length)) {
+    try (BufferAllocator allocator = new RootAllocator(128);
+        ArrowBuf buf = allocator.buffer(expected.length)) {
       buf.setBytes(0, data);
       byte[] actual = new byte[expected.length];
       buf.getBytes(0, actual);
       assertArrayEquals(expected, actual);
     }
   }
 
+  @Test
+  public void testEnabledAssertion() {
+    ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+    ClassLoader.getSystemClassLoader().setDefaultAssertionStatus(true);

Review Comment:
   We probably want to reset this, 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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org