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

HADOOP-16950.Extend Hadoop S3a access from single endpoint to multipl… #1928

Open
wants to merge 1 commit into
base: branch-3.1.3
Choose a base branch
from

Conversation

Ocean-Lua
Copy link

@steveloughran steveloughran self-requested a review March 31, 2020 20:34
Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

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

First, this is a pretty wild idea. Make sense there for things like ceph which tries to offer HA without any load balancer games: the client needs to be clever.

Second: how I review https://github.com/steveloughran/formality/blob/master/styleguide/styleguide.md

We also have a strict "no declared endpoint, no review" policy, as covered in the s3a testing doc. If you are doing something for ceph, I'm going to have to request that you also run against AWS S3 for regression testing.

We also like our s3a tests to have integration tests, with ITest prefix, sorry can verify things actually work with real S3A implementations. This is going to be pretty critical here. At the very least, a list of the same endpoint, repeated, should work for operations against the store.

And, and this might disappoint you, all PRs have to be against hadoop-trunk, currently v 3.4.0.

Regarding the code itself:

cglib & dynamic code generation vs new AmazonS3 implementation,

I really do not want cglib to be used here.

Can't we just wrap the Amazon S3 client instead? We do that for the inconsistent client already, after all.

the risk there is: new methods in later SDKs need to be tracked, but as its a wrap/relay to AmazonS3, were any new methods to be added the wrapper class would fail to compile "Unimplemented Method", we will find out fast. The runbook for an SDK update will need to cover this extra work, but that's all.

Tests

yes, integration tests are mandatory. Especially those exploring failure conditions. Unit tests are nice but not a substitute.

Documentation

That too. Especially extensions to the troubleshooting docs. The easiest way to add new docs would be a whole new markdown file in the site/ dir, with a reference off index.md.

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.

2 participants