HADOOP-17129. Validating storage keys in ABFS correctly #2141

mehakmeet · 2020-07-15T12:37:25Z

Storage Keys in ABFS should be validated after the keys have been loaded.

work:

Remove the previous validation of storage keys.
Validate at the correct place.

Tested by: mvn -T 1C -Dparallel-tests=abfs clean verify
Region: East US, West US.

No tests are required as there are tests already present to validate the patch.

steveloughran

core good; but we need to report that error all the way up

steveloughran · 2020-07-15T13:10:08Z

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/SimpleKeyProvider.java

+    try {
+      validateStorageAccountKey(key);
+    } catch (InvalidConfigurationValueException e) {
+      e.printStackTrace();


Throw it; we want to fail fast rather than wait for auth failures talking to the far end.

Will it be fine to surround the throw new InvalidConfigurationValueException() with one more try-catch block? Since adding an exception to the method would break a test already present.
Also, inside validateStorageAccountKey() we are throwing an InvalidConfigurationValueException() in the validate() method.

Just throw KeyProviderException like it is done in line 49. Or better to call validateStorageAccountKey() just after L48 only so that you don't have to try and catch separately.

I tried to place at L48 as well in the beginning but a test was getting broken, I found at that it expects a certain exception, it passes a null key, which makes the validate() method throw a different error. I think by adding a condition in the catch to throw that particular error if the key is null might solve this. So, I'll be changing it in the next commit.

don't worry about breaking the test, we've moved the vaildation and should expect it. Instead the test will need to handle the way invalid configurations are raised.
Or go with mukund -just make sure that the wrapping exception uses the message of the caught exception and adds it as the cause

Yes, I think the test is fine, I just needed to throw this specific exception when the config value is null which this test is trying to achieve, so I'll throw the exception when it is null else, it'll be like it always has been.

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/SimpleKeyProvider.java

mehakmeet · 2020-07-15T15:59:07Z

Committed 10 mins ago, but can't see the commit here. Is this a known problem?

steveloughran · 2020-07-15T16:27:58Z

you pushed it up?

mehakmeet · 2020-07-15T16:29:15Z

Yes, I can see the commit on my branch, not sure why it's not showing here.

mehakmeet · 2020-07-15T16:40:20Z

Just catch and throw. Why do you need to check again for key==null

Because only for the case where key == null we have to throw this specific Exception, else we can throw the other Exceptions.

mehakmeet · 2020-07-15T16:40:35Z

It seems like the commit is visible now.

mehakmeet · 2020-07-15T17:47:38Z

mehakmeet@c5ab869
3rd commit. Don't know why it's not visible here.

hadoop-yetus · 2020-07-15T19:01:07Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 8s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ trunk Compile Tests _
+1 💚	mvninstall	21m 21s	trunk passed
+1 💚	compile	0m 33s	trunk passed with JDK Ubuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04
+1 💚	compile	0m 29s	trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	checkstyle	0m 20s	trunk passed
+1 💚	mvnsite	0m 32s	trunk passed
+1 💚	shadedclient	16m 24s	branch has no errors when building and testing our client artifacts.
-1 ❌	javadoc	0m 26s	hadoop-azure in trunk failed with JDK Ubuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04.
+1 💚	javadoc	0m 24s	trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+0 🆗	spotbugs	0m 58s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	0m 56s	trunk passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 30s	the patch passed
+1 💚	compile	0m 29s	the patch passed with JDK Ubuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04
+1 💚	javac	0m 29s	the patch passed
+1 💚	compile	0m 24s	the patch passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	javac	0m 24s	the patch passed
+1 💚	checkstyle	0m 15s	the patch passed
+1 💚	mvnsite	0m 26s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedclient	15m 19s	patch has no errors when building and testing our client artifacts.
-1 ❌	javadoc	0m 22s	hadoop-azure in the patch failed with JDK Ubuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04.
+1 💚	javadoc	0m 20s	the patch passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	findbugs	0m 58s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 26s	hadoop-azure in the patch passed.
+1 💚	asflicense	0m 29s	The patch does not generate ASF License warnings.
		65m 29s

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-2141/3/artifact/out/Dockerfile
GITHUB PR	#2141
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux e8f2b962491e 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `1f71c4a`
Default Java	Private Build-1.8.0_252-8u252-b09-1~18.04-b09
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_252-8u252-b09-1~18.04-b09
javadoc	https://builds.apache.org/job/hadoop-multibranch/job/PR-2141/3/artifact/out/branch-javadoc-hadoop-tools_hadoop-azure-jdkUbuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04.txt
javadoc	https://builds.apache.org/job/hadoop-multibranch/job/PR-2141/3/artifact/out/patch-javadoc-hadoop-tools_hadoop-azure-jdkUbuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-2141/3/testReport/
Max. process+thread count	320 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-2141/3/console
versions	git=2.17.1 maven=3.6.0 findbugs=3.1.0-RC1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

mukund-thakur · 2020-07-16T09:10:11Z

LGTM now.

steveloughran · 2020-07-16T16:28:58Z

LGTM
+1: merging to trunk and 3.3

Contributed by Mehakmeet Singh Change-Id: I8016ee2f9ffbc86ea867f4a3d960b134e507d099

HADOOP-17129. Validating storage keys in ABFS correctly

d3b5c40

steveloughran requested changes Jul 15, 2020

View reviewed changes

HADOOP-17129. Fixing review comments

d21076a

HADOOP-17129. Fixing test

c5ab869

mehakmeet requested a review from steveloughran July 16, 2020 06:48

apache deleted a comment from hadoop-yetus Jul 16, 2020

steveloughran merged commit 4083fd5 into apache:trunk Jul 16, 2020

asfgit pushed a commit that referenced this pull request Jul 16, 2020

HADOOP-17129. Validating storage keys in ABFS correctly (#2141)

0d88ed2

Contributed by Mehakmeet Singh Change-Id: I8016ee2f9ffbc86ea867f4a3d960b134e507d099

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HADOOP-17129. Validating storage keys in ABFS correctly #2141

HADOOP-17129. Validating storage keys in ABFS correctly #2141

mehakmeet commented Jul 15, 2020 •

edited

Loading

steveloughran left a comment

steveloughran Jul 15, 2020

mehakmeet Jul 15, 2020 •

edited

Loading

mukund-thakur Jul 15, 2020

mehakmeet Jul 15, 2020

steveloughran Jul 15, 2020 •

edited

Loading

mehakmeet Jul 15, 2020

mehakmeet commented Jul 15, 2020

steveloughran commented Jul 15, 2020

mehakmeet commented Jul 15, 2020

mehakmeet commented Jul 15, 2020

mehakmeet commented Jul 15, 2020 •

edited

Loading

mehakmeet commented Jul 15, 2020

hadoop-yetus commented Jul 15, 2020

mukund-thakur commented Jul 16, 2020

steveloughran commented Jul 16, 2020

HADOOP-17129. Validating storage keys in ABFS correctly #2141

HADOOP-17129. Validating storage keys in ABFS correctly #2141

Conversation

mehakmeet commented Jul 15, 2020 • edited Loading

steveloughran left a comment

Choose a reason for hiding this comment

steveloughran Jul 15, 2020

Choose a reason for hiding this comment

mehakmeet Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

mukund-thakur Jul 15, 2020

Choose a reason for hiding this comment

mehakmeet Jul 15, 2020

Choose a reason for hiding this comment

steveloughran Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

mehakmeet Jul 15, 2020

Choose a reason for hiding this comment

mehakmeet commented Jul 15, 2020

steveloughran commented Jul 15, 2020

mehakmeet commented Jul 15, 2020

mehakmeet commented Jul 15, 2020

mehakmeet commented Jul 15, 2020 • edited Loading

mehakmeet commented Jul 15, 2020

hadoop-yetus commented Jul 15, 2020

mukund-thakur commented Jul 16, 2020

steveloughran commented Jul 16, 2020

mehakmeet commented Jul 15, 2020 •

edited

Loading

mehakmeet Jul 15, 2020 •

edited

Loading

steveloughran Jul 15, 2020 •

edited

Loading

mehakmeet commented Jul 15, 2020 •

edited

Loading