Improve S3, parquet, and SeekableChannelsProvider #5137

devinrsmith · 2024-02-10T00:09:08Z

Fixes #5096

Fixes deephaven#5096

chipkent

Python LGTM. I have not reviewed the rest.

rcaudy

Partial review.

Util/channel/build.gradle

Util/channel/src/main/java/io/deephaven/util/channel/SeekableChannelsProvider.java

Util/channel/src/main/java/io/deephaven/util/channel/SeekableChannelsProviderBase.java

Util/channel/src/main/java/io/deephaven/util/channel/SeekableChannelContextNull.java

Util/channel/src/main/java/io/deephaven/util/channel/SeekableChannelContext.java

Util/channel/src/main/java/io/deephaven/util/channel/ProviderImpl.java

Util/channel/src/main/java/io/deephaven/util/channel/SeekableChannelContext.java

rcaudy · 2024-02-12T22:02:51Z

extensions/s3/src/main/java/io/deephaven/extensions/s3/BufferPool.java

-         * Return the held buffer to its pool, and cause subsequent calls to {@link #get()} to return {@code null}
-         */
-        void close();
+    public ByteBuffer take(final int size) {


I am still somewhat inclined to push for a reference-counted PooledReference<ByteBuffer> or similar, and removing the give method from this class. This let's us guarantee safety when code uses the proper acquire and release pattern.

extensions/parquet/base/src/main/java/io/deephaven/parquet/base/ColumnChunkReaderImpl.java

rcaudy

More partial review

extensions/parquet/base/src/main/java/io/deephaven/parquet/base/ColumnPageReaderImpl.java

extensions/parquet/base/src/main/java/io/deephaven/parquet/base/ParquetFileReader.java

...s/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetColumnLocation.java

...ns/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetTableLocation.java

rcaudy · 2024-02-13T16:56:32Z

extensions/s3/build.gradle

    implementation 'software.amazon.awssdk:s3'
    implementation 'software.amazon.awssdk:aws-crt-client'
+    //implementation 'software.amazon.awssdk:netty-nio-client'


Maybe we should benchmark?

We need better way to benchmark outstanding branches...

Stan has been working on this, but I don't think he has s3 reading benchmarks yet.

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3Instructions.java

rcaudy

I think we can do better than relying on a chain of completable futures for cleanup.

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3SeekableChannelProvider.java

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3SeekableByteChannel.java

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3ChannelContext.java

rcaudy · 2024-02-13T18:30:18Z

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3ChannelContext.java

+
+            @Override
+            public synchronized void onNext(ByteBuffer byteBuffer) {
+                buffer.put(byteBuffer);


This could NPE in the "limbo" case. Do we care?

The likely alternative is to "gracefully" handle when buffer is null, and do localProducer.completeExceptionally. Given I don't expect this to happen, I think hard NPE here is more appropriate (in which case, I expect onError to be called regardless, and that will do the localProducer.completeExceptionally).

I think it depends on whether we're expecting log spam that will scare/annoy users.

Shouldn't be any log spam afaik.

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3ChannelContext.java

Util/channel/src/main/java/io/deephaven/util/channel/LocalFSChannelProvider.java

extensions/parquet/base/src/main/java/io/deephaven/parquet/base/ColumnChunkReaderImpl.java

extensions/parquet/base/src/main/java/io/deephaven/parquet/base/ColumnPageReaderImpl.java

extensions/parquet/base/src/main/java/io/deephaven/parquet/base/ParquetFileReader.java

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3ChannelContext.java

rcaudy

.

rcaudy

.

…annelContext

Base/src/main/java/io/deephaven/base/reference/PooledObjectReference.java

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3ChannelContext.java

Base/src/main/java/io/deephaven/base/reference/PooledObjectReference.java

Base/src/test/java/io/deephaven/base/reference/PooledObjectReferenceTest.java

Improve S3, parquet, and SeekableChannelsProvider

3fa76fd

Fixes deephaven#5096

devinrsmith added NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Feb 10, 2024

devinrsmith added this to the 1. January 2024 (end of month) milestone Feb 10, 2024

devinrsmith requested a review from rcaudy February 10, 2024 00:09

devinrsmith self-assigned this Feb 10, 2024

devinrsmith added 2 commits February 12, 2024 08:13

Anonymous access

11123a7

Merge remote-tracking branch 'upstream/main' into s3-fix-and-refactor

cb45fc5

devinrsmith requested review from chipkent and jmao-denver as code owners February 12, 2024 16:15

Fix decompress call sites

55fef7c

chipkent previously approved these changes Feb 12, 2024

View reviewed changes

rcaudy reviewed Feb 12, 2024

View reviewed changes

rcaudy reviewed Feb 13, 2024

View reviewed changes

Review stuff

beb4aca

devinrsmith dismissed chipkent’s stale review via beb4aca February 14, 2024 01:10

read timeout docs

3a35a56

devinrsmith requested a review from rcaudy February 14, 2024 01:22

rcaudy reviewed Feb 14, 2024

View reviewed changes

small cleanup

f71021f

rcaudy reviewed Feb 14, 2024

View reviewed changes

devinrsmith added 2 commits February 14, 2024 11:08

Slightly better flipping

3f32065

Remove a little bit of Optional alloc

5f0e877

rcaudy reviewed Feb 14, 2024

View reviewed changes

rcaudy and others added 2 commits February 14, 2024 12:53

Port PooledObjectReference, and experimentally integrate it with S3Ch…

897064d

…annelContext

Some updates

edda9a8

devinrsmith requested a review from rcaudy February 14, 2024 21:13

Use acquireIfAvailable

f037b3f

rcaudy reviewed Feb 14, 2024

View reviewed changes

responses

458d29d

devinrsmith requested a review from rcaudy February 14, 2024 21:51

plumb anonymous to python

49dd2c9

devinrsmith requested a review from chipkent February 14, 2024 22:06

rcaudy approved these changes Feb 14, 2024

View reviewed changes

devinrsmith enabled auto-merge (squash) February 14, 2024 22:12

chipkent approved these changes Feb 14, 2024

View reviewed changes

devinrsmith merged commit 8715076 into deephaven:main Feb 14, 2024
18 checks passed

devinrsmith deleted the s3-fix-and-refactor branch February 14, 2024 22:35

github-actions bot locked and limited conversation to collaborators Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve S3, parquet, and SeekableChannelsProvider #5137

Improve S3, parquet, and SeekableChannelsProvider #5137

devinrsmith commented Feb 10, 2024

chipkent left a comment

rcaudy left a comment

rcaudy Feb 12, 2024

rcaudy left a comment

rcaudy Feb 13, 2024

devinrsmith Feb 14, 2024

rcaudy Feb 14, 2024

rcaudy left a comment

rcaudy Feb 13, 2024

devinrsmith Feb 13, 2024

rcaudy Feb 13, 2024

devinrsmith Feb 14, 2024

rcaudy left a comment

rcaudy left a comment

Improve S3, parquet, and SeekableChannelsProvider #5137

Improve S3, parquet, and SeekableChannelsProvider #5137

Conversation

devinrsmith commented Feb 10, 2024

chipkent left a comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcaudy 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

rcaudy 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

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment