Initial draft

-1 overall

This message was automatically generated.

Seems like i have hit a bunch of lines which hadnt been touched in a while.
Ill work on that now.

Thanks for working on this Suraj Acharya. I know this is still in the works, but propose to only log the exception stack trace in debug, to prevent spamming the server logs and unnecessary performance impact.
One way to do that is to log a higher level message without the stack trace, and then log a DEBUG with stacktrace. But since we can already see the error from client side, maybe just DEBUG logging the message + stack trace is fine.

Also ping Arun Suresh for his input.

I agree with the logic of it being a bit too much.
However, my thought process was that since most of these errors will be a fatal error (either killing the operation in progress / killing the startup of the KMS) i thought it was worth putting this as an error message.
But ill take whatever recommendation you'll give me on this one.

I agree those errors usually indicate a problem. But my concern is that, assuming we have 1 problematic key which throws exceptions, we may end up seeing that key log a bunch of stacktraces, hence spamming the log and also impacting server performance. Making it debug can give us more control (e.g. choose to temporarily turn it on on 1 KMS instance etc.)

• Fixed Checkstyle.
• Made the error messages to be debug level

-1 overall

This message was automatically generated.

Thanks Suraj Acharya for revving. Looks good to me. We're just logging a debug log on any exception before throwing. This would make debugging problems a lot easier while not have much impact on normal usages.

Findbugs warnings are false alarm, since the exception is thrown.
+1 pending the below to nits (I'm the human checkstyle wizard):

• Line 198: s/} catch (Exception e){/} catch (Exception e) {/g
• Line 282: s/}catch (Exception e) {/} catch (Exception e) {/g

Fixed human checkstyle issues!

Thanks Xiao Chen.

-1 overall

This message was automatically generated.

Committed this to trunk, branch-2 and branch-2.8.
Thanks Suraj Acharya for the contribution!

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10580 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10580/)
HADOOP-13669. KMS Server should log exceptions before throwing. (xiao: rev 65912e4027548868ebefd8ee36eb00fa889704a7)

• (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java

Findbugs warnings are false alarm, since the exception is thrown.

...

Committed this to trunk, branch-2 and branch-2.8.

Re-opening, because this patch needs an addendum. Please don't commit patches that throw findbugs errors.

If you are sure this is a false alarm, an exception must get added to findbugs-exclude.xml. Until that happens, the release, the nightlies, and all other patches against hadoop-kms are going to fail.

Thanks Allen Wittenauer for reporting this. Sorry for breaking things, I wasn't aware findbug reports have such a severe consequence...

Attaching the addendum. Suraj Acharya, could you take a look?

+1 overall

This message was automatically generated.

Let's trigger the findbugs check to be sure...

-1 overall

This message was automatically generated.

The patch looks good to me.
Sorry about that Allen Wittenauer and Xiao Chen.

============================================================================
                         findbugs detection: trunk
============================================================================
...
hadoop-common-project/hadoop-kms in trunk has 2 extant Findbugs warnings.

============================================================================
                         findbugs detection: patch
============================================================================
============================================================================


cd /testptch/hadoop/hadoop-common-project/hadoop-kms
mvn -Dmaven.repo.local=/home/jenkins/yetus-m2/hadoop-trunk-patch-1 -Ptest-patch -DskipTests test-compile findbugs:findbugs -DskipTests=true > /testptch/hadoop/patchprocess/patch-findbugs-hadoop-common-project_hadoop-kms.txt 2>&1
Elapsed:   0m 21s
Starting with /testptch/hadoop/patchprocess/branch-findbugs-hadoop-common-project_hadoop-kms-warnings.xml
Merging /testptch/hadoop/patchprocess/patch-findbugs-hadoop-common-project_hadoop-kms-warnings.xml
Writing /testptch/hadoop/patchprocess/combined-findbugs-hadoop-common-project_hadoop-kms.xml

The addendum patch fixed the errors. I think this is ready for commit, Allen Wittenauer could you take a look? Thanks a lot.

-1 overall

This message was automatically generated.

Xiao Chen I triggered the rebuild but it picked up the trigger.patch, not the addendum.

Wei-Chiu Chuang thanks looking and kicking a rebuild.
But as you can see from the history, addendum patch didn't trigger the pre-commit for the findbugs. So I added a trigger.patch identical to the addendum fix, plus a line break in KMS.java, so findbugs is run in pre-commit. As a result, findbugs is reporting the trunk version has 2 errors, while the patched version does not have any.

Makes sense?

Got it Xiao Chen
The findbugs warning refers to trunk code, not the patch.

cd /testptch/hadoop/hadoop-common-project/hadoop-kms
mvn -Dmaven.repo.local=/home/jenkins/yetus-m2/hadoop-trunk-patch-0 -Ptest-patch -DskipTests test-compile findbugs:findbugs -DskipTests=true > /testptch/hadoop/patchprocess/branch-findbugs-hadoop-common-project_hadoop-kms.txt 2>&1
Elapsed: 0m 23s

hadoop-common-project/hadoop-kms in trunk has 2 extant Findbugs warnings.

So Yetus should not flag findbugs warning in trunk. Probably worth an improvement.

+1 from me. I'll leave it untouched until EOB in case Allen has additional comments.

Thanks Wei-Chiu Chuang! I'll commit end of today then.

+1 on Yetus should improve on this.. I browsed through Yetus code but didn't find an easy way to fix this. Will create a jira.

Committed the addendum patch to the same branches. Thanks Wei-Chiu Chuang for the review and Allen Wittenauer for raising the issue.

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10609 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10609/)
HADOOP-13669. Addendum patch for KMS Server should log exceptions before (xiao: rev ae51b11f7872eaac558acf00fd23f6d7b1841cfe)

• (edit) hadoop-common-project/hadoop-kms/dev-support/findbugsExcludeFile.xml

Seems to findbugwarnings are still present. Please look the latest jenkins report.

https://builds.apache.org/job/PreCommit-HADOOP-Build/10863/artifact/patchprocess/branch-findbugs-hadoop-common-project_hadoop-kms-warnings.html

https://builds.apache.org/job/hadoop-qbt-trunk-java8-linux-x86/203/

Thanks Brahma Reddy Battula for the report.... pretty weird I verified in intelliJ locally on addendum 1 and it passed. Pre-commit also seemed to agree...
But yes now if I run mvn compile findbugs:findbugs it indeed reports the issue... Attaching an addendum2 patch to fix, also attaching a trigger patch since the fix along won't run the relevant pre-commit.

Marking as patch available such that jenkins will trigger on this.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

The pre-commit -1 is again on the trunk run. For the patched run, we can see this:

============================================================================
============================================================================
                         findbugs detection: patch
============================================================================
============================================================================
....
hadoop-common-project/hadoop-kms generated 0 new + 0 unchanged - 2 fixed = 0 total (was 2)

So it should be fixed this time... Also verified via mvn compile findbugs:findbugs locally.

For the patched run, we can see this:

No need to quote the console. It's in the JIRA message.

I see... It seems I misunderstood the pre-commit on addendum 1. That result was {{+1 findbugs 0m 32s the patch passed}}.. Sorry about that.

Appreciate any reviews on addendum 2. (Is it required? Or is jenkins's +1 suffice here?)

I think you're still missing it:

Pre-process runs and part of it's output is this:

-1 findbugs 0m 26s hadoop-common-project/hadoop-kms in trunk has 2 extant Findbugs warnings.

i.e., trunk has 2 existing findbugs issues

Yetus then starts on it's second pass with the patch in place (the mvninstall lines are good indicators where it is). It then provides this output:

+1 findbugs 0m 34s hadoop-common-project/hadoop-kms generated 0 new + 0 unchanged - 2 fixed = 0 total (was 2)

.... which indicates the patch fixed the 2 findbugs issues.

Thanks for the quick response Allen Wittenauer! (And sorry for not being clear above)

I believe what you described is addendum 2: trunk has 2 existing, patch fixed the 2. So as soon as addendum 2 is reviewed, we can commit... right?

My last comment was for a postmodern on the previously-committed addendum 1. It says trunk has 2 existing, patch passed (but without the explicit (0-2=0) message), so I misunderstood it and thought it fixed the issue.... my bad.

Hi Allen Wittenauer Brahma Reddy Battula Wei-Chiu Chuang,
Could you take a look at addendum2 and see if it makes sense? I'd like to have trunk fixed soon.

Thanks.

Verified addendum2 locally,LGTM,+1..thanks for updating the patch..

Just for reference, For addendum1 <Bug pattern="REC"/> should like <Bug code="REC"/> and along with pattern should be there.

Committed addendum2 to trunk, branch-2 and branch-2.8. Thanks again Brahma Reddy Battula for reporting and reviewing! Also thanks Allen for the feedback.

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10673 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10673/)
HADOOP-13669. Addendum patch 2 for KMS Server should log exceptions (xiao: rev 09ef97dccbd4c11aaeedf7503d414f999435ad91)

• (edit) hadoop-common-project/hadoop-kms/dev-support/findbugsExcludeFile.xml