[GitHub] [arrow] lidavidm commented on a diff in pull request #35314: GH-34723: [Java] Enable log trace for Netty allocator memory usage

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

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


##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java:
##########
@@ -1091,6 +1099,32 @@ public void testMemoryLeakWithReservation() throws Exception {
     }
   }
 
+  @Test
+  public void testMemoryUsage() {
+    ListAppender<ILoggingEvent> memoryLogsAppender = new ListAppender<>();
+    Logger logger = (Logger) LoggerFactory.getLogger("arrow.allocator");
+    logger.setLevel(Level.TRACE);
+    logger.addAppender(memoryLogsAppender);
+    memoryLogsAppender.start();
+    try (ArrowBuf buf = new ArrowBuf(ReferenceManager.NO_OP, null,
+        1024, new PooledByteBufAllocatorL().empty.memoryAddress())) {
+      buf.memoryAddress();
+    }
+    BiFunction<List<String>, Level, Boolean> listAppenderContains =
+        (List<String> values, Level level) -> memoryLogsAppender.list.stream()
+            .anyMatch(
+                log -> log.toString().contains(values.get(0)) &&
+                    log.toString().contains(values.get(1)) &&
+                    log.toString().contains(values.get(2)) &&
+                    log.getLevel().equals(level)
+            );
+    boolean result = listAppenderContains.apply(Arrays.asList("Memory Usage: \n", "Large buffers outstanding: ",
+            "Normal buffers outstanding: "),
+        Level.TRACE);

Review Comment:
   Just write it as a loop...



##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java:
##########
@@ -1091,6 +1099,32 @@ public void testMemoryLeakWithReservation() throws Exception {
     }
   }
 
+  @Test
+  public void testMemoryUsage() {
+    ListAppender<ILoggingEvent> memoryLogsAppender = new ListAppender<>();
+    Logger logger = (Logger) LoggerFactory.getLogger("arrow.allocator");
+    logger.setLevel(Level.TRACE);
+    logger.addAppender(memoryLogsAppender);
+    memoryLogsAppender.start();
+    try (ArrowBuf buf = new ArrowBuf(ReferenceManager.NO_OP, null,
+        1024, new PooledByteBufAllocatorL().empty.memoryAddress())) {
+      buf.memoryAddress();
+    }
+    BiFunction<List<String>, Level, Boolean> listAppenderContains =
+        (List<String> values, Level level) -> memoryLogsAppender.list.stream()
+            .anyMatch(
+                log -> log.toString().contains(values.get(0)) &&
+                    log.toString().contains(values.get(1)) &&
+                    log.toString().contains(values.get(2)) &&
+                    log.getLevel().equals(level)
+            );
+    boolean result = listAppenderContains.apply(Arrays.asList("Memory Usage: \n", "Large buffers outstanding: ",
+            "Normal buffers outstanding: "),
+        Level.TRACE);
+    logger.detachAppender(memoryLogsAppender);

Review Comment:
   this needs to be in a try-finally



##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java:
##########
@@ -1091,6 +1099,32 @@ public void testMemoryLeakWithReservation() throws Exception {
     }
   }
 
+  @Test
+  public void testMemoryUsage() {
+    ListAppender<ILoggingEvent> memoryLogsAppender = new ListAppender<>();
+    Logger logger = (Logger) LoggerFactory.getLogger("arrow.allocator");
+    logger.setLevel(Level.TRACE);

Review Comment:
   Reset the log level afterwards, too



##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java:
##########
@@ -1091,6 +1099,32 @@ public void testMemoryLeakWithReservation() throws Exception {
     }
   }
 
+  @Test
+  public void testMemoryUsage() {
+    ListAppender<ILoggingEvent> memoryLogsAppender = new ListAppender<>();
+    Logger logger = (Logger) LoggerFactory.getLogger("arrow.allocator");
+    logger.setLevel(Level.TRACE);
+    logger.addAppender(memoryLogsAppender);
+    memoryLogsAppender.start();
+    try (ArrowBuf buf = new ArrowBuf(ReferenceManager.NO_OP, null,
+        1024, new PooledByteBufAllocatorL().empty.memoryAddress())) {
+      buf.memoryAddress();

Review Comment:
   And IMO, the easier way to test this would be to factor out a function that generates the log string, test that function, and use that function from the thread, rather than hooking into the physical logger.



##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java:
##########
@@ -1091,6 +1099,32 @@ public void testMemoryLeakWithReservation() throws Exception {
     }
   }
 
+  @Test
+  public void testMemoryUsage() {
+    ListAppender<ILoggingEvent> memoryLogsAppender = new ListAppender<>();
+    Logger logger = (Logger) LoggerFactory.getLogger("arrow.allocator");
+    logger.setLevel(Level.TRACE);
+    logger.addAppender(memoryLogsAppender);
+    memoryLogsAppender.start();
+    try (ArrowBuf buf = new ArrowBuf(ReferenceManager.NO_OP, null,
+        1024, new PooledByteBufAllocatorL().empty.memoryAddress())) {
+      buf.memoryAddress();

Review Comment:
   What I'm saying is, you have no guarantee the logger will actually have logged anything by the time the next few lines run. So this is just adding a flaky test.



-- 
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