Copilot commented on code in PR #9398:
URL: https://github.com/apache/ozone/pull/9398#discussion_r2594132143
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -1113,8 +1078,8 @@ private void dropUnknownColumnFamilies(
List<ColumnFamilyHandle> familyHandles
) {
Set<String> allowedColumnFamilyOnStartUp = new HashSet<>(
- Arrays.asList(DEFAULT_COLUMN_FAMILY_NAME, SNAP_DIFF_JOB_TABLE_NAME,
- SNAP_DIFF_REPORT_TABLE_NAME, SNAP_DIFF_PURGED_JOB_TABLE_NAME));
+ Arrays.asList(DEFAULT_COLUMN_FAMILY_NAME,
SnapshotDiffDBDefinition.SNAP_DIFF_JOB_TABLE_NAME,
+ SnapshotDiffDBDefinition.SNAP_DIFF_REPORT_TABLE_NAME,
SNAP_DIFF_PURGED_JOB_TABLE_NAME));
Review Comment:
[nitpick] Inconsistent constant usage: `SNAP_DIFF_PURGED_JOB_TABLE_NAME` is
statically imported (line 56), while `SNAP_DIFF_JOB_TABLE_NAME` and
`SNAP_DIFF_REPORT_TABLE_NAME` are referenced with the class prefix
`SnapshotDiffDBDefinition`. For consistency, either statically import all three
table name constants or use the class prefix for all of them.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/db/SnapshotDiffMetadataManagerImpl.java:
##########
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.snapshot.db;
+
+import static org.apache.commons.io.FileUtils.deleteDirectory;
+import static org.apache.hadoop.hdds.utils.db.DBStoreBuilder.getDBDirPath;
+import static
org.apache.hadoop.hdds.utils.db.cache.TableCache.CacheType.PARTIAL_CACHE;
+import static
org.apache.hadoop.ozone.om.snapshot.db.SnapshotDiffDBDefinition.SNAP_DIFF_FROM_SNAP_OBJECT_TABLE_DEF;
+import static
org.apache.hadoop.ozone.om.snapshot.db.SnapshotDiffDBDefinition.SNAP_DIFF_JOB_TABLE_DEF;
+import static
org.apache.hadoop.ozone.om.snapshot.db.SnapshotDiffDBDefinition.SNAP_DIFF_PURGED_JOB_TABLE_DEF;
+import static
org.apache.hadoop.ozone.om.snapshot.db.SnapshotDiffDBDefinition.SNAP_DIFF_REPORT_TABLE_NAME_DEF;
+import static
org.apache.hadoop.ozone.om.snapshot.db.SnapshotDiffDBDefinition.SNAP_DIFF_TO_SNAP_OBJECT_TABLE_DEF;
+import static
org.apache.hadoop.ozone.om.snapshot.db.SnapshotDiffDBDefinition.SNAP_DIFF_UNIQUE_IDS_TABLE_DEF;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.hdds.utils.db.DBStore;
+import org.apache.hadoop.hdds.utils.db.DBStoreBuilder;
+import org.apache.hadoop.hdds.utils.db.StringCodec;
+import org.apache.hadoop.hdds.utils.db.Table;
+import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport;
+import org.apache.hadoop.ozone.om.helpers.SnapshotDiffJob;
+import org.apache.hadoop.ozone.om.snapshot.diff.helper.SnapshotDiffObjectInfo;
+
+/**
+ * Implementation of the {@link SnapshotDiffMetadataManager} interface.
+ * Provides functionality for managing metadata related to snapshot difference
+ * operations, including managing tables for snapshot diff jobs, reports,
purged jobs,
+ * and snapshot object information.
+ *
+ * This class handles the persistent storage of snapshot diff metadata using a
database.
+ * It ensures proper initialization and versioning of the metadata schema.
+ */
+public class SnapshotDiffMetadataManagerImpl implements
SnapshotDiffMetadataManager {
+
+ private static final String VERSION_FILE = "VERSION";
+ private static final String VERSION_NUMBER = "1";
+
+ private final DBStore dbStore;
+
+ private final Table<String, SnapshotDiffJob> snapshotDiffJobTable;
+ private final Table<String, SnapshotDiffReport.DiffReportEntry>
snapshotDiffReportTable;
+ private final Table<String, Long> snapshotDiffPurgedJobTable;
+ private final Table<String, SnapshotDiffObjectInfo>
snapshotDiffFromSnapshotObjectInfoTable;
+ private final Table<String, SnapshotDiffObjectInfo>
snapshotDiffToSnapshotObjectInfoTable;
+ private final Table<String, Boolean> snapshotDiffUniqueObjectIdsTable;
+
+ public SnapshotDiffMetadataManagerImpl(ConfigurationSource conf) throws
IOException {
+ File dbPath = getDBDirPath(SnapshotDiffDBDefinition.get(), conf).toPath()
+ .resolve(SnapshotDiffDBDefinition.get().getName()).toFile();
+ Path versionFilePath = dbPath.toPath().resolve(VERSION_FILE);
+ boolean versionContentMatches = checkAndDeleteIfVersionMismatches(dbPath,
versionFilePath);
+
+ this.dbStore = DBStoreBuilder.newBuilder(conf,
SnapshotDiffDBDefinition.get(), dbPath).build();
+
+ this.snapshotDiffJobTable = SNAP_DIFF_JOB_TABLE_DEF.getTable(dbStore,
PARTIAL_CACHE);
+ this.snapshotDiffReportTable =
SNAP_DIFF_REPORT_TABLE_NAME_DEF.getTable(dbStore, PARTIAL_CACHE);
+ this.snapshotDiffPurgedJobTable =
SNAP_DIFF_PURGED_JOB_TABLE_DEF.getTable(dbStore, PARTIAL_CACHE);
+ this.snapshotDiffFromSnapshotObjectInfoTable =
+ SNAP_DIFF_FROM_SNAP_OBJECT_TABLE_DEF.getTable(dbStore, PARTIAL_CACHE);
+ this.snapshotDiffToSnapshotObjectInfoTable =
SNAP_DIFF_TO_SNAP_OBJECT_TABLE_DEF.getTable(dbStore, PARTIAL_CACHE);
+ this.snapshotDiffUniqueObjectIdsTable =
SNAP_DIFF_UNIQUE_IDS_TABLE_DEF.getTable(dbStore, PARTIAL_CACHE);
+ if (!versionContentMatches) {
+ writeVersionFileContent(versionFilePath);
+ }
Review Comment:
The version file is written after the database is created. If
`writeVersionFileContent()` fails (line 82), the database will exist without a
proper version file. On the next startup, this will cause the database to be
deleted (line 102). Consider writing the version file before creating the
database or immediately after in a try-catch block with rollback logic to
ensure consistency.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/helper/SnapshotDiffObjectInfo.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.snapshot.diff.helper;
+
+import
org.apache.hadoop.hdds.protocol.proto.OmServerProtocolProtos.SnapDiffObjectInfo;
+import org.apache.hadoop.hdds.utils.db.Codec;
+import org.apache.hadoop.hdds.utils.db.DelegatedCodec;
+import org.apache.hadoop.hdds.utils.db.Proto2Codec;
+
+/**
+ * Represents information about an object in a snapshot difference.
+ * This class provides methods for converting between its internal
+ * representation and a Protobuf-based representation using a custom
+ * serialization codec.
+ */
+public class SnapshotDiffObjectInfo {
+
+ private static final Codec<SnapshotDiffObjectInfo> CODEC =
+ new
DelegatedCodec<>(Proto2Codec.get(SnapDiffObjectInfo.getDefaultInstance()),
+ SnapshotDiffObjectInfo::getFromProtobuf,
+ SnapshotDiffObjectInfo::toProtobuf,
+ SnapshotDiffObjectInfo.class);
+
+ private long objectId;
+ private String key;
+
+ public SnapshotDiffObjectInfo(long objectId, String key) {
+ this.objectId = objectId;
+ this.key = key;
+ }
+
Review Comment:
The SnapshotDiffObjectInfo class is missing getter methods for its fields
(objectId and key). While the class currently only needs to support
serialization/deserialization, adding getters would make it more useful when
the stored data needs to be read and used by other code.
```suggestion
/**
* Returns the object ID.
*/
public long getObjectId() {
return objectId;
}
/**
* Returns the key name.
*/
public String getKey() {
return key;
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/db/SnapshotDiffDBDefinition.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.snapshot.db;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OM_SNAPSHOT_DIFF_DB_NAME;
+import static
org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_SNAPSHOT_DIFF_DB_DIR;
+import static
org.apache.hadoop.ozone.snapshot.SnapshotDiffReportOzone.getDiffReportEntryCodec;
+
+import java.util.Map;
+import org.apache.hadoop.hdds.utils.db.BooleanCodec;
+import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition;
+import org.apache.hadoop.hdds.utils.db.DBDefinition;
+import org.apache.hadoop.hdds.utils.db.LongCodec;
+import org.apache.hadoop.hdds.utils.db.StringCodec;
+import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport.DiffReportEntry;
+import org.apache.hadoop.ozone.om.helpers.SnapshotDiffJob;
+import org.apache.hadoop.ozone.om.snapshot.diff.helper.SnapshotDiffObjectInfo;
+
+/**
+ * The SnapshotDiffDBDefinition class defines the schema for the snapshot
+ * difference database tables used in snapshot diff operations. Each table
+ * corresponds to a specific aspect of snapshot diff job processing and data
+ * storage, ensuring proper organization and efficient access.
+ */
+public final class SnapshotDiffDBDefinition extends DBDefinition.WithMap {
+ /**
+ * Contains all the snap diff job which are either queued, in_progress or
+ * done. This table is used to make sure that there is only single job for
+ * requests with the same snapshot pair at any point of time.
+ * |------------------------------------------------|
+ * | KEY | VALUE |
+ * |------------------------------------------------|
+ * | fromSnapshotId-toSnapshotId | SnapshotDiffJob |
+ * |------------------------------------------------|
+ */
+ public static final String SNAP_DIFF_JOB_TABLE_NAME = "snap-diff-job-table";
+
+ public static final DBColumnFamilyDefinition<String, SnapshotDiffJob>
SNAP_DIFF_JOB_TABLE_DEF
+ = new DBColumnFamilyDefinition<>(SNAP_DIFF_JOB_TABLE_NAME,
StringCodec.get(), SnapshotDiffJob.getCodec());
+ /**
+ * Global table to keep the diff report. Each key is prefixed by the jobId
+ * to improve look up and clean up. JobId comes from snap-diff-job-table.
+ * |--------------------------------|
+ * | KEY | VALUE |
+ * |--------------------------------|
+ * | jobId-index | DiffReportEntry |
+ * |--------------------------------|
+ */
+ public static final String SNAP_DIFF_REPORT_TABLE_NAME =
"snap-diff-report-table";
+ public static final DBColumnFamilyDefinition<String, DiffReportEntry>
SNAP_DIFF_REPORT_TABLE_NAME_DEF
+ = new DBColumnFamilyDefinition<>(SNAP_DIFF_REPORT_TABLE_NAME,
StringCodec.get(), getDiffReportEntryCodec());
+ /**
+ * Contains all the snap diff job which can be purged either due to max
+ * allowed time is over, FAILED or REJECTED.
+ * |-------------------------------------------|
+ * | KEY | VALUE |
+ * |-------------------------------------------|
+ * | jobId | numOfTotalEntriesInReportTable |
+ * |-------------------------------------------|
+ */
+ public static final String SNAP_DIFF_PURGED_JOB_TABLE_NAME =
"snap-diff-purged-job-table";
+ public static final DBColumnFamilyDefinition<String, Long>
SNAP_DIFF_PURGED_JOB_TABLE_DEF
+ = new DBColumnFamilyDefinition<>(SNAP_DIFF_PURGED_JOB_TABLE_NAME,
StringCodec.get(), LongCodec.get());
+ /**
+ * Contains all the snap diff job intermediate object output for from
snapshot.
+ * |----------------------------------------------------|
+ * | KEY | VALUE |
+ * |----------------------------------------------------|
+ * | jobId-objectId | SnapshotDiffObjectInfo |
+ * |----------------------------------------------------|
+ */
+ public static final String SNAP_DIFF_FROM_SNAP_OBJECT_TABLE_NAME =
"-from-snap";
+ public static final DBColumnFamilyDefinition<String, SnapshotDiffObjectInfo>
SNAP_DIFF_FROM_SNAP_OBJECT_TABLE_DEF
+ = new DBColumnFamilyDefinition<>(SNAP_DIFF_FROM_SNAP_OBJECT_TABLE_NAME,
StringCodec.get(),
+ SnapshotDiffObjectInfo.getCodec());
+
+ /**
+ * Contains all the snap diff job intermediate object output for to snapshot.
+ * |----------------------------------------------------|
+ * | KEY | VALUE |
+ * |----------------------------------------------------|
+ * | jobId-objectId | SnapshotDiffObjectInfo |
+ * |----------------------------------------------------|
+ */
+ public static final String SNAP_DIFF_TO_SNAP_OBJECT_TABLE_NAME = "-to-snap";
+ public static final DBColumnFamilyDefinition<String, SnapshotDiffObjectInfo>
SNAP_DIFF_TO_SNAP_OBJECT_TABLE_DEF
+ = new DBColumnFamilyDefinition<>(SNAP_DIFF_TO_SNAP_OBJECT_TABLE_NAME,
StringCodec.get(),
+ SnapshotDiffObjectInfo.getCodec());
+
+ /**
+ * Contains all the snap diff job intermediate object output for to snapshot.
Review Comment:
The documentation comment incorrectly states "Contains all the snap diff job
intermediate object output for to snapshot." This should describe what the
unique IDs table actually stores (e.g., "Contains unique object IDs encountered
during snapshot diff job processing.").
```suggestion
* Contains unique object IDs encountered during snapshot diff job
processing.
* Each entry maps a jobId-objectId pair to a boolean indicating whether
the
* object ID is unique for the given job.
```
##########
hadoop-hdds/interface-server/src/main/proto/OmServerProtocol.proto:
##########
@@ -0,0 +1,37 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * These .proto interfaces are private and unstable.
+ * Please see http://wiki.apache.org/hadoop/Compatibility
+ * for what changes are allowed for a *unstable* .proto interface.
+ */
+syntax = "proto2";
+option java_package = "org.apache.hadoop.hdds.protocol.proto";
+option java_outer_classname = "OmServerProtocolProtos";
+option java_generic_services = true;
+option java_generate_equals_and_hash = true;
+package hadoop.hdds.block;
Review Comment:
The proto package `hadoop.hdds.block` seems inconsistent with the file name
`OmServerProtocol.proto` and the comment "// OM Server protocol". Since this is
related to OM (Ozone Manager) server protocol, consider using a more
appropriate package name like `hadoop.hdds.om` or `hadoop.hdds.ozone.om`
instead of `hadoop.hdds.block`.
```suggestion
package hadoop.hdds.om;
```
--
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]
