Ran across this while working with leveldb as part of MAPREDUCE-5652 and YARN-1336. There are two DBExceptions, NativeDB.DBException and leveldb.DBException. The former is derived from IOException raised by the low level JNI code, while the latter is derived from RuntimeException and is thrown by the JniDB wrapper code. To make matters worse, DBIterator throws raw RuntimeException rather than the runtime DBException from its methods, so database errors can leak up the stack even if code is expecting the runtime DBException.

The timeline store should be handling the runtime exceptions and treat them like I/O errors, at least to keep it from tearing down the deletion thread (if not other cases).

We may want to create a wrapper utility class for DBIterator in YARN as a workaround so interacting with the database only requires handling of leveldb.DBException rather than also trying to wrestle with the raw RuntimeExceptions from the iterator. See the DBIterator wrapper class in https://issues.apache.org/jira/secure/attachment/12641927/MAPREDUCE-5652-v8.patch as a rough example.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12683190/YARN-1984.patch
against trunk revision a4df9ee.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/5906//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/5906//console

This message is automatically generated.

Thanks for picking this up, Varun.

getStartTimeLong can leak the runtime DBException and shouldn't.

Is there a reason to have deleteNextEntity throw DBException rather than IOException? It would be cleaner for callers if deleteNextEnttiy handled this.

loadVersion can leak the runtime DBException

Thanks for the review Jason Lowe.

I had missed handling DBException at one place and other places didnt handle because the caller method was handling DBException.
But in hindsight, I think we should handle the DBException in all the methods you mentioned above, as the method signature doesn't advertise throwing of DBException.

I have uploaded a new patch. Kindly review.

Thanks for your effort, Varun and Jason!

Is there a reason to have deleteNextEntity throw DBException rather than IOException? It would be cleaner for callers if deleteNextEnttiy handled this.

Maybe we don't need to to that. It's consistent to catch DBException at the same method where LeveldbIterator is constructed, but not at the inner method where LeveldbIterator is passed in. The test case should be fine if we need to catch DBException separately when testing a private method.

loadVersion can leak the runtime DBException

It seems that loadVersion doesn't user iterator. Or LeveldbIterator can help the get method too?

BTW, handleException can be static and more general to taken one more param: error code, such that it can be reused in more places of this class.

Thanks Zhijie Shen for the review.

loadVersion needs to handle DBException because DB#get can throw DBException.
I guess handling DBException inside deleteNextEntity is a matter of choice. But as the method advertises throwing only IOException, handling DBException inside the method would avoid any mistakes in future if a developer chooses to call this method and overlooks handling DBException.

You are correct. handleException can be changed to static and take one more error code. Will upload a patch with these changes.

Made the changes as per review. Kindly review Jason Lowe and Zhijie Shen

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12683371/YARN-1984.001.patch
against trunk revision 555fa2d.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/5918//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/5918//console

This message is automatically generated.

discardOldEntites does not need to handle DBException if deleteNextEntity handles it. Updated the patch with this change.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12683377/YARN-1984.002.patch
against trunk revision 555fa2d.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/5919//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/5919//console

This message is automatically generated.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12683388/YARN-1984.002.patch
against trunk revision f636f9d.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/5921//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/5921//console

This message is automatically generated.

+1 latest patch lgtm. Zhijie Shen do you have further comments?

LGTM

Thanks to Varun for the contribution and to Zhijie for additional review! I committed this to trunk and branch-2.

FAILURE: Integrated in Hadoop-trunk-Commit #6597 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6597/)
YARN-1984. LeveldbTimelineStore does not handle db exceptions properly. Contributed by Varun Saxena (jlowe: rev 1ce4d33c2dc86d711b227a04d2f9a2ab696a24a1)

• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/TestLeveldbTimelineStore.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/timeline/LeveldbTimelineStore.java

FAILURE: Integrated in Hadoop-Yarn-trunk #754 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/754/)
YARN-1984. LeveldbTimelineStore does not handle db exceptions properly. Contributed by Varun Saxena (jlowe: rev 1ce4d33c2dc86d711b227a04d2f9a2ab696a24a1)

• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/TestLeveldbTimelineStore.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/timeline/LeveldbTimelineStore.java

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/16/)
YARN-1984. LeveldbTimelineStore does not handle db exceptions properly. Contributed by Varun Saxena (jlowe: rev 1ce4d33c2dc86d711b227a04d2f9a2ab696a24a1)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/TestLeveldbTimelineStore.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/timeline/LeveldbTimelineStore.java
• hadoop-yarn-project/CHANGES.txt

SUCCESS: Integrated in Hadoop-Hdfs-trunk #1944 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1944/)
YARN-1984. LeveldbTimelineStore does not handle db exceptions properly. Contributed by Varun Saxena (jlowe: rev 1ce4d33c2dc86d711b227a04d2f9a2ab696a24a1)

• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/TestLeveldbTimelineStore.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/timeline/LeveldbTimelineStore.java

SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/16/)
YARN-1984. LeveldbTimelineStore does not handle db exceptions properly. Contributed by Varun Saxena (jlowe: rev 1ce4d33c2dc86d711b227a04d2f9a2ab696a24a1)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/TestLeveldbTimelineStore.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/timeline/LeveldbTimelineStore.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/16/)
YARN-1984. LeveldbTimelineStore does not handle db exceptions properly. Contributed by Varun Saxena (jlowe: rev 1ce4d33c2dc86d711b227a04d2f9a2ab696a24a1)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/TestLeveldbTimelineStore.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/timeline/LeveldbTimelineStore.java
• hadoop-yarn-project/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #1968 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1968/)
YARN-1984. LeveldbTimelineStore does not handle db exceptions properly. Contributed by Varun Saxena (jlowe: rev 1ce4d33c2dc86d711b227a04d2f9a2ab696a24a1)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/TestLeveldbTimelineStore.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/timeline/LeveldbTimelineStore.java
• hadoop-yarn-project/CHANGES.txt

Pulled this into 2.6.1 as a dependency for YARN-2952. Ran compilation and TestLeveldbTimelineStore before the push. Patch applied cleanly.