Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(Files): Add integration tests for Files.copy / read* #252

Merged
merged 7 commits into from
Nov 6, 2023

Conversation

guicamest
Copy link
Contributor

@guicamest guicamest commented Nov 2, 2023

Issue #236

Description of changes:

As discussed in the issue, a non-crt asyncclient will be used for reading files through the S3ReadAheadByteChannel. This is the class used when using the nio api via Files.copy or Files.read*.

Some private methods have been renamed to better reflect the purpose of them.

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

@guicamest guicamest changed the title test(Files): Add integration tests for Files.copy / read* (issue #236) test(Files): Add integration tests for Files.copy / read* Nov 3, 2023
@guicamest guicamest force-pushed the issue-236-load-files branch 2 times, most recently from abfe16f to 91d5b2f Compare November 3, 2023 14:18
@guicamest guicamest marked this pull request as ready for review November 4, 2023 21:19
@guicamest
Copy link
Contributor Author

While doing the change I came across universalClient method(s), I haven't found a purpose for them and thought of cleaning them out. @markjschreiber would that be ok?

}

return client;
}

S3AsyncClient readClient() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you wouldn't just overload client() with client(boolean crtClient)? Either way, it is probably worth documenting why a CRT client is not used in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client() method saves a reference of that client in the current class. Unlike client(), readClient() creates a new client every time a read needs to be done. This client is later closed when the readChannel is close(). I think it'd be clearer to have them in separate methods for now, one is a general purpose and the other one is only for reads.

Copy link
Contributor

@markjschreiber markjschreiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor question. Otherwise looking good.

@markjschreiber markjschreiber merged commit 642dade into awslabs:main Nov 6, 2023
1 check passed
@guicamest guicamest deleted the issue-236-load-files branch November 6, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants