devmadhuu commented on code in PR #11560:
URL: https://github.com/apache/ozone/pull/11560#discussion_r4297056481
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/recovery/ReconOmMetadataManagerImpl.java:
##########
@@ -153,6 +152,13 @@ public void updateOmDB(File newDbLocation, boolean
addCacheMetrics) throws IOExc
DBStore current = getStore();
try {
initializeNewRdbStore(newDbLocation, addCacheMetrics);
+ } catch (IOException e) {
+ if (current == getStore()) {
+ // Do not leave a snapshot that failed to open behind as the last
known OM DB.
+ LOG.warn("Deleting OM snapshot db at {} that could not be opened.",
newDbLocation.getAbsolutePath());
+ FileUtils.deleteQuietly(newDbLocation);
+ }
+ throw e;
Review Comment:
After this is thrown from here, it will be caught in
`updateReconOmDBWithNewSnapshot` and then this method will return as `false` in
`executeFullSnapshot`, now there won't be any retry for fullsnapshot again till
any delta events and process failure. Till then, Recon stays on a DB whose
folder is gone. `syncDataFromOM` only does a full snapshot when the sequence
number is ≤ 0 or a delta fails. So need to handle that.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/recovery/ReconOmMetadataManagerImpl.java:
##########
@@ -121,13 +125,8 @@ public void start(OzoneConfiguration configuration) throws
IOException {
* @param addCacheMetrics
*/
private void initializeNewRdbStore(File dbFile, boolean addCacheMetrics)
throws IOException {
- try {
- setStore(DBStoreBuilder.newBuilder(ozoneConfiguration,
OMDBDefinition.get(), dbFile).build());
- LOG.info("Created OM DB handle from snapshot at {}.",
- dbFile.getAbsolutePath());
- } catch (IOException ioEx) {
- LOG.error("Unable to initialize Recon OM DB snapshot store.", ioEx);
- }
+ setStore(DBStoreBuilder.newBuilder(ozoneConfiguration,
OMDBDefinition.get(), dbFile).build());
+ LOG.info("Created OM DB handle from snapshot at {}.",
dbFile.getAbsolutePath());
if (getStore() != null) {
initializeOmTables(TableCache.CacheType.FULL_CACHE, addCacheMetrics);
Review Comment:
Failure can happen here or in `setStore` call, its not differentiated. So
lets say if `setStore` call is succeeded, but `initializeOmTables` throw error,
then `current == getStore()` condition will be false and recon has switched to
new OM DB snapshot, but since initialize table call failed, some API calls for
recon will continue to read stale data silently without knowing. Also dangerous
part is that subsequent all delta operations will continue to process from that
last updated sequence number and will update further events and updates to OM
tables pointing to old DB tables as `initializeOmTables` call was failed. This
also will happen silently.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
