ArafatKhan2198 commented on code in PR #11494:
URL: https://github.com/apache/ozone/pull/11494#discussion_r4256274016
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ReconContainerMetadataManagerImpl.java:
##########
@@ -241,11 +244,13 @@ public void batchStoreContainerReplicaHistory(
Map<Long, Map<DatanodeID, ContainerReplicaHistory>> replicaHistoryMap)
throws IOException {
try (BatchOperation batchOperation =
- containerDbStore.initBatchOperation()) {
+ replicaHistoryDBProvider.getDbStore().initBatchOperation()) {
for (Map.Entry<Long, Map<DatanodeID, ContainerReplicaHistory>> entry :
replicaHistoryMap.entrySet()) {
final long containerId = entry.getKey();
- final Map<DatanodeID, ContainerReplicaHistory> tsMap =
entry.getValue();
+ // The in-memory map contains only current replicas; preserve departed
datanodes on shutdown.
+ final Map<DatanodeID, ContainerReplicaHistory> tsMap =
getContainerReplicaHistory(containerId);
+ tsMap.putAll(entry.getValue());
Review Comment:
For a datanode present in both maps, `putAll()` replaces the entire stored
history record, including `firstSeenTime`.
For example, if the stored record says the replica was first seen on Monday,
and the post-restart in-memory record starts on Wednesday, shutdown changes the
stored first-seen time to Wednesday.
Could we preserve the earliest `firstSeenTime` while updating the latest
replica information, and add a test using the same datanode with different
stored and in-memory timestamps?
This issue existed before this PR, so a separate follow-up Jira is also
reasonable.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ReconReplicaHistoryDBDefinition.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.recon.spi.impl;
+
+import static
org.apache.hadoop.ozone.recon.ReconConstants.RECON_REPLICA_HISTORY_DB;
+import static
org.apache.hadoop.ozone.recon.spi.impl.ReconDBDefinition.REPLICA_HISTORY_V2;
+
+import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition;
+import org.apache.hadoop.hdds.utils.db.DBDefinition;
+import org.apache.hadoop.hdds.utils.db.StringCodec;
+import org.apache.hadoop.ozone.recon.ReconServerConfigKeys;
+
+/**
+ * Replica history is independent of the OM-derived tables rebuilt during
reprocess.
+ */
+public class ReconReplicaHistoryDBDefinition extends DBDefinition.WithMap {
+ // Stores the migration completion marker in the destination DB so a retry
can skip copying history
+ // if the copy succeeded but persisting Recon's upgrade version failed.
+ public static final DBColumnFamilyDefinition<String, String> METADATA =
+ new DBColumnFamilyDefinition<>("metadata", StringCodec.get(),
StringCodec.get());
+
+ public ReconReplicaHistoryDBDefinition() {
+ super(DBColumnFamilyDefinition.newUnmodifiableMap(REPLICA_HISTORY_V2,
METADATA));
+ }
+
+ @Override
+ public String getName() {
+ return RECON_REPLICA_HISTORY_DB;
Review Comment:
Could we also register this new database in `DBDefinitionFactory`?
The factory does not recognize `recon-container-replica-history.db`, so
running `ozone debug ldb --db <history-db-path> scan --cf replica_history_v2`
will report “Incorrect DB Path”, even with a valid path. Previously, operators
could inspect this history through the recognized container-key database.
Please register `ReconReplicaHistoryDBDefinition` and extend
`TestDBDefinitionFactory` to cover the new database and its column families.
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/scm/TestReconStorageContainerManagerFacade.java:
##########
@@ -90,6 +92,10 @@ void testScmSnapshotDbIsOpenedAtCanonicalReconPath() throws
Exception {
ReconStorageContainerManagerFacade reconScm =
injector.getInstance(ReconStorageContainerManagerFacade.class);
+ ReconContainerManager containerManager = (ReconContainerManager)
reconScm.getContainerManager();
+ DatanodeID historyDatanode = DatanodeID.randomID();
+ containerManager.upsertContainerHistory(1L, historyDatanode, 10L, 20L,
Review Comment:
One existing issue remains in the replica-removal path:
`upsertContainerHistory()` catches `IOException` and returns, after which
`removeContainerReplica()` removes the entry from memory regardless. A failed
database write can therefore discard history information that was not saved.
Could we propagate the failure or return a success status, and remove the
in-memory entry only after persistence succeeds? Similarly, the shutdown flush
should retain the map when its batch write fails.
A test that forces the write to throw `IOException` should verify that the
in-memory history remains available. This can be tracked separately since the
behavior predates this PR.
--
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]
