[GitHub] [spark] tgravescs commented on a diff in pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method

[GitBox <gi...@apache.org>]

tgravescs commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967137328


##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
     return tmpDb;
   }
 
-  @VisibleForTesting

Review Comment:
   so as @mridulm said in the previous review this does add a bit of behavior change where we could hide a corruption that likely would have failed the test before.  It would seem like that is a pretty rare case though and the test wasn't specifically trying to test that.   
   
   Honestly I can go either way on this change, I'm ok with it but at the same time don't think these extra  functions are much maintenance unless you had other reasons for removing?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org