hemantk-12 commented on code in PR #4418:
URL: https://github.com/apache/ozone/pull/4418#discussion_r1145405582
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java:
##########
@@ -77,6 +77,8 @@ public class RDBStore implements DBStore {
* Can be made configurable later.
*/
private final String dbCompactionLogDirName = "compaction-log";
+ // Can't import OmMetadataManagerImpl (circular dependency otherwise)
Review Comment:
Suggestion: Can it be move to `OzoneConsts`?
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -400,13 +403,58 @@ public void setRocksDBForCompactionTracking(DBOptions
rocksOptions)
setRocksDBForCompactionTracking(rocksOptions, new ArrayList<>());
}
+ /**
+ * Set SnapshotInfoTable DB column family handle to be used in DB listener.
+ * @param snapshotInfoTableCFHandle ColumnFamilyHandle
+ */
+ public void setSnapshotInfoTableCFHandle(
+ ColumnFamilyHandle snapshotInfoTableCFHandle) {
+ Preconditions.checkNotNull(snapshotInfoTableCFHandle,
+ "Column family handle should not be null");
+ this.snapshotInfoTableCFHandle = snapshotInfoTableCFHandle;
+ }
+
+ /**
+ * Helper method to check whether the SnapshotInfoTable column family is
empty
+ * in a given DB instance.
+ * @param db RocksDB instance
+ * @return true when column family is empty, false otherwise
+ */
+ private boolean isSnapshotInfoTableEmpty(RocksDB db) {
+ // Can't use metadataManager.getSnapshotInfoTable().isEmpty() or use
+ // any wrapper classes here. Any of those introduces circular dependency.
+ // The solution here is to use raw RocksDB API.
+
+ // There is this small gap when the db is open but the handle is not yet
set
+ // in RDBStore. Compaction could theoretically happen during that small
+ // window. This condition here aims to handle that (falls back to not
+ // skipping compaction tracking).
+ if (snapshotInfoTableCFHandle == null) {
+ LOG.warn("Snapshot info table column family handle is not set!");
+ // Proceed to log compaction in this case
+ return false;
+ }
+
+ // SnapshotInfoTable has table cache, but it wouldn't matter in this case.
Review Comment:
1. I can't think of anything other than multiple override for `snapshotInfo`
entry in the `snapshotInfoTable` iin DB as you mentioned, assuming that it has
unlimited retry in case of failure. Also if another snapshot is taken that
would be queued on top of this one.
2. I'm not sure if it would work in this case but you can use helper class
and move "SnapshotInfoTable table cache check" to it to remove the cyclic
dependency.
--
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]
