You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "davisusanibar (via GitHub)" <gi...@apache.org> on 2023/06/13 04:35:47 UTC
[GitHub] [arrow] davisusanibar opened a new pull request, #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
davisusanibar opened a new pull request, #36042:
URL: https://github.com/apache/arrow/pull/36042
### Rationale for this change
To close #34338.
### What changes are included in this PR?
Removing the automatic enabling of BaseAllocator.DEBUG on -ea
### Are these changes tested?
Unit test added.
### Are there any user-facing changes?
Yes.
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245934449
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -165,14 +166,14 @@ public void testEnabledHistoricalLog() {
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) {
+ allocator.buffer(2);
+ Exception e = assertThrows(IllegalStateException.class, () -> allocator.close());
assertTrue(e.getMessage().contains("event log for:")); // JDK8, JDK11
} finally {
fieldDebug.set(null, false);
}
} catch (Exception e) {
- assertTrue(e.toString().contains("java.lang.NoSuchFieldException: modifiers")); // JDK17+
+ assertTrue(e.toString().equals("java.lang.NoSuchFieldException: modifiers")); // JDK17+
Review Comment:
Why was this changed to equals and not contains?
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245474790
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,52 @@ 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
+ /**
+ * Test that allocation history is not recorded even though
+ * assertions are enabled in tests (GH-34338).
+ */
+ public void testEnabledAssertion() {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+ try (BufferAllocator allocator = new RootAllocator(128)) {
+ ArrowBuf buf = allocator.buffer(2);
+ } catch (Exception e) {
+ assertFalse(e.getMessage().contains("event log for:"));
+ } finally {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(null);
+ }
+ }
+
+ @Test
+ public void testEnabledHistoricalLog() {
Review Comment:
It seems this is the wrong way to test this given that it won't work on newer Java versions. We should investigate a better way to test this (a separate test suite that runs with different JVM parameters, I suppose)
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,52 @@ 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
+ /**
+ * Test that allocation history is not recorded even though
+ * assertions are enabled in tests (GH-34338).
+ */
+ public void testEnabledAssertion() {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+ try (BufferAllocator allocator = new RootAllocator(128)) {
+ ArrowBuf buf = allocator.buffer(2);
+ } catch (Exception e) {
+ assertFalse(e.getMessage().contains("event log for:"));
Review Comment:
Can we use assertThrows
--
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
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245916896
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,52 @@ 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
+ /**
+ * Test that allocation history is not recorded even though
+ * assertions are enabled in tests (GH-34338).
+ */
+ public void testEnabledAssertion() {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+ try (BufferAllocator allocator = new RootAllocator(128)) {
+ ArrowBuf buf = allocator.buffer(2);
+ } catch (Exception e) {
+ assertFalse(e.getMessage().contains("event log for:"));
Review Comment:
Changed
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1228336602
##########
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:
Don't we need to reset it to its original value, instead of assuming what it was?
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245966612
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -165,14 +166,14 @@ public void testEnabledHistoricalLog() {
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) {
+ allocator.buffer(2);
+ Exception e = assertThrows(IllegalStateException.class, () -> allocator.close());
assertTrue(e.getMessage().contains("event log for:")); // JDK8, JDK11
} finally {
fieldDebug.set(null, false);
}
} catch (Exception e) {
- assertTrue(e.toString().contains("java.lang.NoSuchFieldException: modifiers")); // JDK17+
+ assertTrue(e.toString().equals("java.lang.NoSuchFieldException: modifiers")); // JDK17+
Review Comment:
It's rather brittle
--
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
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245928459
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,52 @@ 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
+ /**
+ * Test that allocation history is not recorded even though
+ * assertions are enabled in tests (GH-34338).
+ */
+ public void testEnabledAssertion() {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+ try (BufferAllocator allocator = new RootAllocator(128)) {
+ ArrowBuf buf = allocator.buffer(2);
+ } catch (Exception e) {
+ assertFalse(e.getMessage().contains("event log for:"));
+ } finally {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(null);
+ }
+ }
+
+ @Test
+ public void testEnabledHistoricalLog() {
Review Comment:
We could decouple our test that need to override system property but at the end the unique way to inject a parameter will be by reflection.
By definition there is not way to share system properties between main code and test code, for that reason what other framework do are using reflection to change that at runtime.
The initial scope of this test is JDK11, for another JDK17+ versions this test validate only the message 'java.lang.NoSuchFieldException: modifiers'
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245966857
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,52 @@ 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
+ /**
+ * Test that allocation history is not recorded even though
+ * assertions are enabled in tests (GH-34338).
+ */
+ public void testEnabledAssertion() {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+ try (BufferAllocator allocator = new RootAllocator(128)) {
+ ArrowBuf buf = allocator.buffer(2);
+ } catch (Exception e) {
+ assertFalse(e.getMessage().contains("event log for:"));
+ } finally {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(null);
+ }
+ }
+
+ @Test
+ public void testEnabledHistoricalLog() {
Review Comment:
So the test becomes worthless on JDK17+. I would rather we find a proper way to test this instead of a brittle test that will silently become invalid over time.
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1228541911
##########
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:
How does this test even work, actually? The property is already set by the time you get here.
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1228543100
##########
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:
I think you're better off asserting that assertions are enabled (as they will be during a test) and that the property is not set.
--
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
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1228936637
##########
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:
Ohhh thank you, you are right, assertions are enabled by default in all the unit test scope, then I remove the property and only validate the message.
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "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
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1228524962
##########
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:
Added
--
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
[GitHub] [arrow] lidavidm commented on pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #36042:
URL: https://github.com/apache/arrow/pull/36042#issuecomment-1591290518
@davisusanibar can you look into the flaky build? This has happened a lot.
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1229511402
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,48 @@ 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() {
Review Comment:
```suggestion
/**
* Test that allocation history is not recorded even though
* assertions are enabled in tests (GH-34338).
*/
public void testEnabledAssertion() {
```
--
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
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245960719
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -165,14 +166,14 @@ public void testEnabledHistoricalLog() {
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) {
+ allocator.buffer(2);
+ Exception e = assertThrows(IllegalStateException.class, () -> allocator.close());
assertTrue(e.getMessage().contains("event log for:")); // JDK8, JDK11
} finally {
fieldDebug.set(null, false);
}
} catch (Exception e) {
- assertTrue(e.toString().contains("java.lang.NoSuchFieldException: modifiers")); // JDK17+
+ assertTrue(e.toString().equals("java.lang.NoSuchFieldException: modifiers")); // JDK17+
Review Comment:
To be more accurately
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36042:
URL: https://github.com/apache/arrow/pull/36042#issuecomment-1588528031
:warning: GitHub issue #34338 **has been automatically assigned in GitHub** to PR creator.
--
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
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245979042
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -165,14 +166,14 @@ public void testEnabledHistoricalLog() {
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) {
+ allocator.buffer(2);
+ Exception e = assertThrows(IllegalStateException.class, () -> allocator.close());
assertTrue(e.getMessage().contains("event log for:")); // JDK8, JDK11
} finally {
fieldDebug.set(null, false);
}
} catch (Exception e) {
- assertTrue(e.toString().contains("java.lang.NoSuchFieldException: modifiers")); // JDK17+
+ assertTrue(e.toString().equals("java.lang.NoSuchFieldException: modifiers")); // JDK17+
Review Comment:
Ok, let me maintain contains instead.
--
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
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1246762670
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,52 @@ 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
+ /**
+ * Test that allocation history is not recorded even though
+ * assertions are enabled in tests (GH-34338).
+ */
+ public void testEnabledAssertion() {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+ try (BufferAllocator allocator = new RootAllocator(128)) {
+ ArrowBuf buf = allocator.buffer(2);
+ } catch (Exception e) {
+ assertFalse(e.getMessage().contains("event log for:"));
+ } finally {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(null);
+ }
+ }
+
+ @Test
+ public void testEnabledHistoricalLog() {
Review Comment:
Just filled a ticket https://github.com/apache/arrow/issues/36390
--
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
[GitHub] [arrow] lidavidm merged pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #36042:
URL: https://github.com/apache/arrow/pull/36042
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1246540560
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,52 @@ 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
+ /**
+ * Test that allocation history is not recorded even though
+ * assertions are enabled in tests (GH-34338).
+ */
+ public void testEnabledAssertion() {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+ try (BufferAllocator allocator = new RootAllocator(128)) {
+ ArrowBuf buf = allocator.buffer(2);
+ } catch (Exception e) {
+ assertFalse(e.getMessage().contains("event log for:"));
+ } finally {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(null);
+ }
+ }
+
+ @Test
+ public void testEnabledHistoricalLog() {
Review Comment:
Can you file a ticket?
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36042:
URL: https://github.com/apache/arrow/pull/36042#discussion_r1245967118
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java:
##########
@@ -138,12 +129,52 @@ 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
+ /**
+ * Test that allocation history is not recorded even though
+ * assertions are enabled in tests (GH-34338).
+ */
+ public void testEnabledAssertion() {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(Level.TRACE);
+ try (BufferAllocator allocator = new RootAllocator(128)) {
+ ArrowBuf buf = allocator.buffer(2);
+ } catch (Exception e) {
+ assertFalse(e.getMessage().contains("event log for:"));
+ } finally {
+ ((Logger) LoggerFactory.getLogger("org.apache.arrow")).setLevel(null);
+ }
+ }
+
+ @Test
+ public void testEnabledHistoricalLog() {
Review Comment:
That doesn't have to block this PR, but I don't consider this test as a proper validation of the debug mode.
--
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
[GitHub] [arrow] davisusanibar commented on pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on PR #36042:
URL: https://github.com/apache/arrow/pull/36042#issuecomment-1612442353
Just filled a ticket for Cyclonedx errors: https://github.com/apache/arrow/issues/36371
--
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
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36042: GH-34338: [Java] Removing the automatic enabling of BaseAllocator.DEBUG on -ea
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36042:
URL: https://github.com/apache/arrow/pull/36042#issuecomment-1620805006
Conbench analyzed the 6 benchmark runs on commit `8c113425`.
There were 2 benchmark results indicating a performance regression:
- Commit Run on `ursa-thinkcentre-m75q` at [2023-07-03 01:11:19Z](http://conbench.ursa.dev/compare/runs/237b048541ce4094b083bd3b1305b68c...b280132314494331a746d5db2aec1ff8/)
- [params=<FloatType>, source=cpp-micro, suite=parquet-bloom-filter-benchmark](http://conbench.ursa.dev/compare/benchmarks/064a1f6b43ac77468000f8610d1dfffb...064a220f7bac7069800008277d8daa1a)
- Commit Run on `arm64-m6g-linux-compute` at [2023-06-29 18:15:15Z](http://conbench.ursa.dev/compare/runs/d8d3d3ee8d9d40458dea5d40521898e7...d9dd5d633ee9475495a6528c5a9dfc14/)
- [params=num_cols:64/is_partial:1/real_time, source=cpp-micro, suite=arrow-ipc-read-write-benchmark](http://conbench.ursa.dev/compare/benchmarks/0649dbed4b317bf480009438c20417a1...0649dca82fa3797680006ec88d585bc5)
The [full Conbench report](https://github.com/apache/arrow/runs/14778497255) has more details.
--
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