Extending unit test coverage and fixing race condition #45

IsaevIlya · 2024-06-13T14:03:47Z

Description of changes:
Extend code coverage to return code coverage back to 95%. Especially by adding multi thread test for running SeekaleInputStream in concurrent environment.
Update access pattern for data owned by AutoClosingCircularBuffer. Move logic for looking up for appropriate IOBlock from MultiObjectsBlockManager inside AutoClosingCircularBuffer and wrap it with sync block. That is need to prevent concurrent exception that happens when Thread A is iterating through content of AutoClosingCircularBuffer and Tread B is modifying it at the same time. That cause iterator used by Thread A to become invalid.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ahmarsuhail

initial feedback, not reviewed test code yet

input-stream/src/main/java/com/amazon/connector/s3/S3SeekableInputStream.java

input-stream/src/main/java/com/amazon/connector/s3/io/logical/impl/ParquetLogicalIOImpl.java

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/BlockManagerConfiguration.java

input-stream/src/main/java/com/amazon/connector/s3/io/physical/blockmanager/IOBlock.java

...src/main/java/com/amazon/connector/s3/io/physical/blockmanager/MultiObjectsBlockManager.java

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java

…toClosingCircularBuffer and moving reading of IOBlock inside it.

…tests

… safe

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java

...est/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBufferTest.java

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java

radhisat · 2024-06-14T15:04:56Z

input-stream/src/test/java/com/amazon/connector/s3/S3SeekableInputStreamTest.java

+                  PhysicalIO physicalIO = new PhysicalIOImpl(new BlockManager(blockManager, s3URI));
+                  LogicalIO logicalIO =
+                      new ParquetLogicalIOImpl(physicalIO, LogicalIOConfiguration.DEFAULT);
+                  SeekableInputStream stream = new S3SeekableInputStream(logicalIO);


Can we also have multiple threads trying to prefetch at the same time?

Could you elaborate what do you mean? My expectation was that each SeekableInputStream with default configuration will issue request to prefetch footer.

the minute you initiate S3SeekableInputStream, it'll kick off the tail prefetch

👍 My bad, the default configuration is to prefetch the tail

radhisat

Looks good. Left some minor comments

ahmarsuhail

Thank you!!! the tests are great!

I just left some nits. but all good otherwise

ahmarsuhail · 2024-06-14T15:10:43Z

input-stream/src/main/java/com/amazon/connector/s3/io/physical/blockmanager/IOBlock.java

-    for (int i = 0; i < len; i++) {
-      buff[i] = blockContent[positionToOffset(pos) + i];
-    }
+  public int read(byte[] buff, int offset, long len, long pos) {


ahmarsuhail · 2024-06-14T15:12:23Z

...src/main/java/com/amazon/connector/s3/io/physical/blockmanager/MultiObjectsBlockManager.java

@@ -255,6 +246,8 @@ private IOBlock createBlock(long start, long end, S3URI s3URI, boolean isPrefetc
                .key(s3URI.getKey())
                .range(new Range(OptionalLong.of(start), OptionalLong.of(end)))
                .build());
+
+    LOG.info("Creating IOBlock {}:{} for {}", start, end, s3URI);


nit: instead of S3URI, should we just use s3URI.getKey() in our logs? I think that might make it easier to parse

ahmarsuhail · 2024-06-14T15:18:29Z

input-stream/src/test/java/com/amazon/connector/s3/S3SeekableInputStreamTest.java

+                  PhysicalIO physicalIO = new PhysicalIOImpl(new BlockManager(blockManager, s3URI));
+                  LogicalIO logicalIO =
+                      new ParquetLogicalIOImpl(physicalIO, LogicalIOConfiguration.DEFAULT);
+                  SeekableInputStream stream = new S3SeekableInputStream(logicalIO);


the minute you initiate S3SeekableInputStream, it'll kick off the tail prefetch

ahmarsuhail · 2024-06-14T15:19:59Z

input-stream/src/test/java/com/amazon/connector/s3/S3SeekableInputStreamTest.java

+        e.printStackTrace();
+      }
+    }
+    boolean result = haveException.get();


super nit: rename variable to "completedWithException"
and then do `assertFalse(completedWithException, )

ahmarsuhail · 2024-06-14T15:21:34Z

input-stream/src/test/java/com/amazon/connector/s3/S3SeekableInputStreamTest.java

+            new FakeObjectClient(sb.toString()), BlockManagerConfiguration.DEFAULT);
+    AtomicBoolean haveException = new AtomicBoolean(false);
+
+    // Create 20 threads to start multiple SeekableInputStream to read last 4 bytes


nit: update comment, we are also reading first 4 bytes.

also to confirm, when our IoBlock was not immutable, this reproduced the issue right?

Yes. It was throwing one type of exception before changes in IOBlock - BufferUnderflowException. After changes in IOBlock, it start to throw ConcurrentModificationException.

ahmarsuhail · 2024-06-14T15:21:46Z

input-stream/src/test/java/com/amazon/connector/s3/S3SeekableInputStreamTest.java

@@ -307,4 +332,50 @@ void testMinusOneIsHandledProperly() throws IOException {
    assertEquals(-1, stream.read(b, 0, LEN));
    assertEquals(INITIAL_POS, stream.getPos());
  }
+
+  @Test
+  void testMultiThreadUsage() throws IOException, InterruptedException {


really like this test, thanks!

ahmarsuhail · 2024-06-14T15:23:48Z

...t-stream/src/test/java/com/amazon/connector/s3/io/logical/impl/ParquetLogicalIOImplTest.java

+
+  @Test
+  void testMetadaWithNegativeContentLength() {
+    // FakeObjectClient fakeObjectClient = new FakeObjectClient("");


nit: remove commented code

ahmarsuhail · 2024-06-14T15:27:35Z

input-stream/src/test/java/com/amazon/connector/s3/io/physical/blockmanager/IOBlockTest.java

+
+    int one = '1';
+    int zero = '0';
+    assertEquals(one, ioBlock.getByte(0 + offset));


sorry this test is a bit hard to follow for me, could you please explain what we are testing here exactly?

This test is checking:

we are correctly handling blocks that has start offset.

we are returning exactly those bytes that were requested without messing with indexes

if we receive request to read bytes outside of current block we throw excetpion

ahmarsuhail

+1, merging

IsaevIlya force-pushed the iiseav/code-coverage branch 2 times, most recently from 832c108 to 651bf7f Compare June 13, 2024 15:20

ahmarsuhail requested changes Jun 14, 2024

View reviewed changes

IsaevIlya force-pushed the iiseav/code-coverage branch from 651bf7f to 1da9112 Compare June 14, 2024 10:18

radhisat reviewed Jun 14, 2024

View reviewed changes

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java Outdated Show resolved Hide resolved

radhisat reviewed Jun 14, 2024

View reviewed changes

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java Outdated Show resolved Hide resolved

radhisat reviewed Jun 14, 2024

View reviewed changes

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java Outdated Show resolved Hide resolved

IsaevIlya force-pushed the iiseav/code-coverage branch from 65088bd to cee95e9 Compare June 14, 2024 11:17

IsaevIlya requested a review from ahmarsuhail June 14, 2024 11:17

IsaevIlya added 3 commits June 14, 2024 12:39

Extending unit test coverage

3c39484

Add syncronizations to prevent race condition on iterating through Au…

d4db509

…toClosingCircularBuffer and moving reading of IOBlock inside it.

Add more test to improve coverage and update formatting for existing …

92c622b

…tests

IsaevIlya force-pushed the iiseav/code-coverage branch from cee95e9 to 1e265ef Compare June 14, 2024 11:40

Address first feedbacks and modify FakeObjectClient to be more thread…

5988c66

… safe

IsaevIlya force-pushed the iiseav/code-coverage branch from 1e265ef to 5988c66 Compare June 14, 2024 12:36

radhisat reviewed Jun 14, 2024

View reviewed changes

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java Show resolved Hide resolved

radhisat reviewed Jun 14, 2024

View reviewed changes

...est/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBufferTest.java Show resolved Hide resolved

radhisat reviewed Jun 14, 2024

View reviewed changes

...rc/main/java/com/amazon/connector/s3/io/physical/blockmanager/AutoClosingCircularBuffer.java Outdated Show resolved Hide resolved

radhisat reviewed Jun 14, 2024

View reviewed changes

Add resources clearing under sync block

4288ad2

radhisat previously approved these changes Jun 14, 2024

View reviewed changes

Add TODO with ticket link for commented part of unit test

4f4ae78

IsaevIlya dismissed radhisat’s stale review via 4f4ae78 June 14, 2024 15:15

ahmarsuhail requested changes Jun 14, 2024

View reviewed changes

Address PR comments

8c6187b

radhisat approved these changes Jun 14, 2024

View reviewed changes

ahmarsuhail approved these changes Jun 14, 2024

View reviewed changes

ahmarsuhail merged commit c469147 into awslabs:main Jun 14, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extending unit test coverage and fixing race condition #45

Extending unit test coverage and fixing race condition #45

IsaevIlya commented Jun 13, 2024 •

edited

Loading

ahmarsuhail left a comment

radhisat Jun 14, 2024

IsaevIlya Jun 14, 2024

ahmarsuhail Jun 14, 2024

radhisat Jun 14, 2024

radhisat left a comment

ahmarsuhail left a comment

ahmarsuhail Jun 14, 2024

ahmarsuhail Jun 14, 2024

ahmarsuhail Jun 14, 2024

ahmarsuhail Jun 14, 2024

ahmarsuhail Jun 14, 2024

IsaevIlya Jun 14, 2024

ahmarsuhail Jun 14, 2024

ahmarsuhail Jun 14, 2024

ahmarsuhail Jun 14, 2024

IsaevIlya Jun 14, 2024

ahmarsuhail left a comment

Extending unit test coverage and fixing race condition #45

Extending unit test coverage and fixing race condition #45

Conversation

IsaevIlya commented Jun 13, 2024 • edited Loading

ahmarsuhail left a 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

radhisat left a comment

Choose a reason for hiding this comment

ahmarsuhail left a 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

ahmarsuhail left a comment

Choose a reason for hiding this comment

IsaevIlya commented Jun 13, 2024 •

edited

Loading