HADOOP-16998 WASB : NativeAzureFsOutputStream#close() throwing java.l… #2073

anoopsjohn · 2020-06-13T17:50:40Z

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

ramkrish86

Non binding.

liuml07

+1

nit: you may want to fix the checkstyle warnings, @anoopsjohn

Will keep it open this week so @steveloughran may take a look.

ayushtkn

+1

steveloughran

suggested some changes. Also add a try-with-resources implicit close() wrapping a failing close(); that's where abfs + gzip was having problems

steveloughran · 2020-06-17T13:49:31Z

...op-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/SyncableDataOutputStream.java


 import org.apache.hadoop.fs.StreamCapabilities;
 import org.apache.hadoop.fs.Syncable;
+import org.slf4j.Logger;


put in an import block just above the org.apache one. I know a lot of classes don't do that -but that is because the move from o.a.commons.logging to SLF4J was done with 'sed'

Done.
org.apache.hadoop.classification.InterfaceAudience was wrongly placed. Corrected it now.

sorry, i meant "org.slf4j to go into a block above the org.apache one", below the java. one...its where we like keep the non-apache, non-static imports

...ools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestSyncableDataOutputStream.java

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

liuml07 · 2020-06-21T07:24:16Z

javadoc error is not related but checkstyle warning is related?

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

anoopsjohn · 2020-06-21T12:20:20Z

javadoc error is not related but checkstyle warning is related?

Checkstyle is reported in test class
./hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestSyncableDataOutputStream.java:44: boolean flushThrowIOE = false;:13: Variable 'flushThrowIOE' must be private and have accessor methods. [VisibilityModifier]

So checkstyle applicable for Test classes also in Hadoop? This is my first patch here and so not really aware. Anyways I have pushed a new commit with private modifier. This was a static inner class in Test class so easy thing.

steveloughran

code is good, just those imports. Imports are a PITA as they create so much merge conflict.

in new code, current layout is

java.*
javax.*

all non static but org apache

org.apache.*

import static

``

existing code: keep the diff to a minimum, no reorder, etc, just to keep cherrypicking and merging of multiple patches viable

other than that: all good to go

steveloughran · 2020-06-22T13:01:28Z

...op-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/SyncableDataOutputStream.java


 import org.apache.hadoop.fs.StreamCapabilities;
 import org.apache.hadoop.fs.Syncable;
+import org.slf4j.Logger;


sorry, i meant "org.slf4j to go into a block above the org.apache one", below the java. one...its where we like keep the non-apache, non-static imports

steveloughran · 2020-06-22T13:02:42Z

...ools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestSyncableDataOutputStream.java

+import java.io.OutputStream;
+
+import org.apache.hadoop.test.LambdaTestUtils;
+import org.junit.Test;


again, org.junit to a (new) "non org.apache imports' block

Thanks Steve for explaining..
Sorry I did the organize import at eclipse level and just used the same import formatter what using for hbase.
Did the 2 import order changes as u suggested and pushed.

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

steveloughran

call me fussy for being strict about comments but for all the places where we have "false" merge conflict it's there. I wish we had spark's 'reject all patches with the wrong settings' check -but it is too late now. instead we do it by hand, and have to expect it to decay. What we can do is start right and hope to retain

please look at the previous discusssion and the ordering I discussed there.

steveloughran · 2020-06-23T13:30:48Z

...op-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/SyncableDataOutputStream.java


+import org.apache.hadoop.fs.StreamCapabilities;
+import org.apache.hadoop.fs.Syncable;
+import org.apache.hadoop.classification.InterfaceAudience;


we are getting so close here. Now move this up to L28.

You mean move to L27 so that org.slf4j and org.apche are single block?

sorry Steve am not getting fully.
So you say move this entire org.apache block to L27 (So that new new line between org.slf4j and org.apache block)
Or u want to move import org.apache.hadoop.classification.InterfaceAudience; (In L30) to L28 so that within org.apache block things are properly imported in order and still keep this org.apache and org.slf as 2 blocks.

liuml07 · 2020-07-06T23:23:52Z

This is still open. @steveloughran do you have a chance to look at the latest code and also question @anoopsjohn has? Thanks,

anoopsjohn · 2020-07-07T05:18:05Z

Sorry I got confused with the order of imports that we follow in Hadoop.. Do we have a code formatter for Eclipse which specify the import order?
Based on the line number what Steve mentioned, I believe he mean this
"Or u want to move import org.apache.hadoop.classification.InterfaceAudience; (In L30) to L28 so that within org.apache block things are properly imported in order and still keep this org.apache and org.slf as 2 blocks"

Let me anyways change that in my PR.

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

swapped the final two import blocks

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

merged to trunk; rebuilding branch 3.3 with the patch cherrypicked.. not quite set up to run wasb tests there so just validating that the patch went in OK

hadoop-yetus · 2020-07-14T14:06:10Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 33s	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	19m 21s	trunk passed
+1 💚	compile	0m 37s	trunk passed with JDK Ubuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04
+1 💚	compile	0m 32s	trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	checkstyle	0m 25s	trunk passed
+1 💚	mvnsite	0m 36s	trunk passed
+1 💚	shadedclient	14m 59s	branch has no errors when building and testing our client artifacts.
-1 ❌	javadoc	0m 29s	hadoop-azure in trunk failed with JDK Ubuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04.
+1 💚	javadoc	0m 27s	trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+0 🆗	spotbugs	0m 55s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	0m 53s	trunk passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 29s	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 25s	the patch passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	javac	0m 25s	the patch passed
+1 💚	checkstyle	0m 17s	the patch passed
+1 💚	mvnsite	0m 27s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedclient	13m 41s	patch has no errors when building and testing our client artifacts.
-1 ❌	javadoc	0m 25s	hadoop-azure in the patch failed with JDK Ubuntu-11.0.7+10-post-Ubuntu-2ubuntu218.04.
+1 💚	javadoc	0m 23s	the patch passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	findbugs	0m 55s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 28s	hadoop-azure in the patch passed.
+1 💚	asflicense	0m 32s	The patch does not generate ASF License warnings.
		60m 33s

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-2073/6/artifact/out/Dockerfile
GITHUB PR	#2073
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux b147b6dfa804 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `48f9011`
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-2073/6/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-2073/6/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-2073/6/testReport/
Max. process+thread count	414 (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-2073/6/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.

…alArgumentException (#2073) Contributed by Anoop Sam John.

HADOOP-16998 WASB : NativeAzureFsOutputStream#close() throwing java.l…

717c870

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

liuml07 requested review from liuml07 and steveloughran June 13, 2020 19:00

liuml07 assigned anoopsjohn Jun 13, 2020

liuml07 added the fs/azure changes related to azure; submitter must declare test endpoint label Jun 13, 2020

ramkrish86 approved these changes Jun 16, 2020

View reviewed changes

liuml07 approved these changes Jun 16, 2020

View reviewed changes

ayushtkn approved these changes Jun 17, 2020

View reviewed changes

steveloughran requested changes Jun 17, 2020

View reviewed changes

HADOOP-16998 WASB : NativeAzureFsOutputStream#close() throwing java.l…

b021879

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

HADOOP-16998 WASB : NativeAzureFsOutputStream#close() throwing java.l…

570eca1

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

apache deleted a comment from hadoop-yetus Jun 22, 2020

steveloughran requested changes Jun 22, 2020

View reviewed changes

HADOOP-16998 WASB : NativeAzureFsOutputStream#close() throwing java.l…

fa29d02

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

steveloughran requested changes Jun 23, 2020

View reviewed changes

oss-maker approved these changes Jun 26, 2020

View reviewed changes

anoopsjohn and others added 2 commits July 7, 2020 10:50

HADOOP-16998 WASB : NativeAzureFsOutputStream#close() throwing java.l…

d72aa7e

…ang.IllegalArgumentException instead of IOE which causes HBase RS to get aborted.

Update TestSyncableDataOutputStream.java

e0a8551

swapped the final two import blocks

steveloughran approved these changes Jul 14, 2020

View reviewed changes

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

steveloughran merged commit 380e0f4 into apache:trunk Jul 14, 2020

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

HADOOP-16998. WASB : NativeAzureFsOutputStream#close() throwing Illeg…

cac2fc1

…alArgumentException (#2073) Contributed by Anoop Sam John.

steveloughran mentioned this pull request Jul 5, 2021

HADOOP-17788. Replace IOUtils#closeQuietly usages by Hadoop's own utility #3171

Merged

ayushtkn mentioned this pull request Sep 13, 2021

HADOOP-17892. Add Hadoop code formatter in dev-support #3387

Merged

HADOOP-16998 WASB : NativeAzureFsOutputStream#close() throwing java.l… #2073

HADOOP-16998 WASB : NativeAzureFsOutputStream#close() throwing java.l… #2073

Uh oh!

Conversation

anoopsjohn commented Jun 13, 2020

Uh oh!

ramkrish86 left a comment

Choose a reason for hiding this comment

Uh oh!

liuml07 left a comment

Choose a reason for hiding this comment

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liuml07 commented Jun 21, 2020

Uh oh!

anoopsjohn commented Jun 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuml07 commented Jul 6, 2020

Uh oh!

anoopsjohn commented Jul 7, 2020

Uh oh!

steveloughran commented Jul 14, 2020

Uh oh!

hadoop-yetus commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

anoopsjohn commented Jun 21, 2020 •

edited

Loading