-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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-18830. Cut S3 Select #6144
HADOOP-18830. Cut S3 Select #6144
Conversation
Given I wrote this it's kind of sad, but openFile() will outlive it as it has many more uses. |
🎊 +1 overall
This message was automatically generated. |
Tested s3 london |
with this PR
Retained
I have a concern about retention #2; it uses the unshaded package we may want to cut it -we can always add a ref in the package docs to say its' been cut and which git commit to go to so as to find it. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@@ -35,7 +35,6 @@ full details. | |||
|
|||
* [Encryption](./encryption.html) | |||
* [Performance](./performance.html) | |||
* [S3Guard](./s3guard.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we removing this? think the S3Guard page is still there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think i moved it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sad to see this go too..it complicated the SDK V2 work and we had to add in a lot of code to get this work.
Looks good, few things:
- Remove the CsvFile class?
- In S3Utils we have SC_405_METHOD_NOT_ALLOWED, comment says "seen on S3 Select" . maybe we don't need that anymore
- Couple of open todos in
ITestSelectUnsupported
And agree with your comment, should cut |
keeping but moving csv file. it's actually very useful in places; in cloudstore we use it in places like "bandwidth" to track block upload times. so it may be useful in future...leaving it in test/ means there's no commitment to maintain |
616e55a
to
a4346dd
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
a4346dd
to
88f202d
Compare
🎊 +1 overall
This message was automatically generated. |
8029a13
to
eadbd95
Compare
latest change is just a rebase; not retested |
💔 -1 overall
This message was automatically generated. |
eadbd95
to
a254800
Compare
rebased pr with retest. failures unrelated; the signing one has an active pr to fix, the committer one looks like my config is at fault (bucket overrides not being cut)
|
🎊 +1 overall
This message was automatically generated. |
@ahmarsuhail @mukund-thakur can I get this in? it'll help on test time... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. just a couple of javadoc nits.
@@ -17,7 +17,12 @@ | |||
*/ | |||
|
|||
/** | |||
* Support for S3 Select. | |||
* Was the location for support for S3 Select. | |||
* Now removed apart from some constants.f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo
" WHERE s._1 = 'foo'"; | ||
|
||
/** | ||
* A {@code .must(SELECT_SQL, _)} option MSUT not fail the build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo in MUST
and this isn't clear to me, .must should fail and throw Unsupported Exception right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct. updated this and other javadocs, named methods appropriately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM.
💔 -1 overall
This message was automatically generated. |
thanks. not sure what is up with docker. will rebase and force push with test run |
Cut out S3 Select * leave public/unstable constants alone * s3guard tool will fail with error * s3afs. path capability will fail * openFile() will fail with specific error * s3 select doc updated * Cut eventstream jar * New test: ITestSelectUnsupported verifies new failure handling above Everything else cut, including tests. Change-Id: Iff8c1e6562ed2d47abefe2cfef2909c00c78a55d
+update doc with explicit consequences of removal, including the "hadoop s3guard select" command Change-Id: Ica3ffb33d803f8a03e0d733447e4f026d547f8c1
1ff7c34
to
736a3cc
Compare
🎊 +1 overall
This message was automatically generated. |
Review in docs/core-siten references to landsat and CSV; update as appropriate. This includes * A review of use of .endpoint vs .endpoint.region and a move to fs.s3a.endpoint.region as much as possible. * reorg of some sections * fixup of a name= headers. Change-Id: I5e07cdf153cacf0ce1ee6673d3a094c2d5eaf5a2
💔 -1 overall
This message was automatically generated. |
Cut out S3 Select * leave public/unstable constants alone * s3guard tool will fail with error * s3afs. path capability will fail * openFile() will fail with specific error * s3 select doc updated * Cut eventstream jar * New test: ITestSelectUnsupported verifies new failure handling above Contributed by Steve Loughran
Cut out S3 Select * leave public/unstable constants alone * s3guard tool will fail with error * s3afs. path capability will fail * openFile() will fail with specific error * s3 select doc updated * New test: ITestSelectUnsupported verifies new failure handling above This is the v1 SDK version of the feature. Contributed by Steve Loughran Change-Id: Ic18c49562e5143a2a2204d66840be149db486b02
Cut out S3 Select * leave public/unstable constants alone * s3guard tool will fail with error * s3afs. path capability will fail * openFile() will fail with specific error * s3 select doc updated * New test: ITestSelectUnsupported verifies new failure handling above This is the v1 SDK version of the feature. Contributed by Steve Loughran Change-Id: Ic18c49562e5143a2a2204d66840be149db486b02
Cut out S3 Select * leave public/unstable constants alone * s3guard tool will fail with error * s3afs. path capability will fail * openFile() will fail with specific error * s3 select doc updated * Cut eventstream jar * New test: ITestSelectUnsupported verifies new failure handling above Contributed by Steve Loughran
Cut out S3 Select * leave public/unstable constants alone * s3guard tool will fail with error * s3afs. path capability will fail * openFile() will fail with specific error * s3 select doc updated * New test: ITestSelectUnsupported verifies new failure handling above This is the v1 SDK version of the change. Contributed by Steve Loughran
Cut out S3 Select * leave public/unstable constants alone * s3guard tool will fail with error * s3afs. path capability will fail * openFile() will fail with specific error * s3 select doc updated * Cut eventstream jar * New test: ITestSelectUnsupported verifies new failure handling above Contributed by Steve Loughran
Cut out S3 Select
Everything else cut, including tests.
New test: ITestSelectUnsupported
verifies new failure handling above
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?