smengcl commented on code in PR #4418:
URL: https://github.com/apache/ozone/pull/4418#discussion_r1144116194
##########
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:
@hemantk-12 Do you think if it is reasonable to move
`createOmSnapshotCheckpoint()` call right after adding the snapshot entry to DB
`putWithBatch`? Looks to me it should only have the consequence of adding the
extra snapshotInfo entry in the snapshotInfoTable in the checkpoint DB which
doesn't really matter.
https://github.com/apache/ozone/blob/c7de8d5871209a86e79f8422c6334697b32f113a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotCreateResponse.java#L68-L77
This could avoid the necessity of checking table cache inside the the
listener. (Introduces cyclic dependency as soon as I introduces `Table` class
`org.apache.hadoop.hdds.utils.db.Table`, unless we do some refactoring surgery
to cleanly separate that part out.)
Would it break any assumptions we had that I hadn't thought of?
##########
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:
@hemantk-12 Do you think if it is reasonable to move
`createOmSnapshotCheckpoint()` call right after adding the snapshot entry to DB
`putWithBatch`? Looks to me it should only have the consequence of adding the
extra snapshotInfo entry in the snapshotInfoTable in the checkpoint DB which
doesn't really matter.
https://github.com/apache/ozone/blob/c7de8d5871209a86e79f8422c6334697b32f113a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotCreateResponse.java#L68-L77
This could avoid the necessity of checking table cache inside the the
listener. (Will introduce cyclic dependency as soon as I introduces `Table`
class `org.apache.hadoop.hdds.utils.db.Table`, unless we do some refactoring
surgery to cleanly separate that part out.)
Would it break any assumptions we had that I hadn't thought of?
--
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]
