You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/06/02 15:47:05 UTC
[GitHub] [ozone] smengcl commented on a diff in pull request #3468: HDDS-6783. Recon tasks should write in batches to their databases
smengcl commented on code in PR #3468:
URL: https://github.com/apache/ozone/pull/3468#discussion_r888047138
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/ReconContainerMetadataManager.java:
##########
@@ -190,4 +225,7 @@ void deleteContainerMapping(ContainerKeyPrefix containerKeyPrefix)
*/
void incrementContainerCountBy(long count);
+ void commitBatchOperation(RDBBatchOperation rdbBatchOperation)
Review Comment:
nit: javadoc
```
/**
* Commit a batch operation into the containerDbStore.
*
* @param rdbBatchOperation batch operation we want to commit
*/
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/ReconContainerMetadataManager.java:
##########
@@ -53,6 +55,17 @@ void reinitWithNewContainerDataFromOm(Map<ContainerKeyPrefix, Integer>
void storeContainerKeyMapping(ContainerKeyPrefix containerKeyPrefix,
Review Comment:
Since we use `batchStoreContainerKeyMapping()` now, and this `storeContainerKeyMapping()` is only used by a unit test (`TestReconContainerMetadataManagerImpl`), should we add a line in the javadoc to discourage others from using this non-batch version method (or even mark `@Deprecated` for now or remove it later)? Same for `batchStoreContainerKeyCounts()` and others.
And we should probably change the unit tests (e.g. in `TestReconContainerMetadataManagerImpl`) to test the batch version as well.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ReconNamespaceSummaryManagerImpl.java:
##########
@@ -66,6 +75,11 @@ public NSSummary getNSSummary(long objectId) throws IOException {
return nsSummaryTable.get(objectId);
}
+ public void commitBatchOperation(RDBBatchOperation rdbBatchOperation)
Review Comment:
nit: missing `@Override`.
The `@Override` annotation is here optional but it is a good practice to add it to prevent simple mistakes. See https://stackoverflow.com/a/212624
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/ReconContainerMetadataManager.java:
##########
@@ -190,4 +225,7 @@ void deleteContainerMapping(ContainerKeyPrefix containerKeyPrefix)
*/
void incrementContainerCountBy(long count);
+ void commitBatchOperation(RDBBatchOperation rdbBatchOperation)
Review Comment:
nit: javadoc
```
/**
* Commit a batch operation into the containerDbStore.
*
* @param rdbBatchOperation batch operation we want to commit
*/
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ContainerKeyMapperTask.java:
##########
@@ -156,19 +178,70 @@ public Pair<String, Boolean> process(OMUpdateEventBatch events) {
return new ImmutablePair<>(getTaskName(), false);
}
}
+ try {
+ writeToTheDB(containerKeyMap, containerKeyCountMap, deletedKeyCountList);
+ } catch (IOException e) {
+ LOG.error("Unable to write Container Key Prefix data in Recon DB.", e);
+ return new ImmutablePair<>(getTaskName(), false);
+ }
LOG.info("{} successfully processed {} OM DB update event(s).",
getTaskName(), eventCount);
return new ImmutablePair<>(getTaskName(), true);
}
+ private void writeToTheDB(Map<ContainerKeyPrefix, Integer> containerKeyMap,
+ Map<Long, Long> containerKeyCountMap,
+ List<ContainerKeyPrefix> deletedContainerKeyList)
+ throws IOException {
+ RDBBatchOperation rdbBatchOperation = new RDBBatchOperation();
+ containerKeyMap.keySet().forEach((ContainerKeyPrefix key) -> {
+ try {
+ reconContainerMetadataManager
+ .batchStoreContainerKeyMapping(rdbBatchOperation, key,
+ containerKeyMap.get(key));
+ } catch (IOException e) {
+ LOG.error("Unable to write Container Key Prefix data in Recon DB.",
+ e);
+ }
+ });
+
+ containerKeyCountMap.keySet().forEach((Long key) -> {
+ try {
+ reconContainerMetadataManager
+ .batchStoreContainerKeyCounts(rdbBatchOperation, key,
+ containerKeyCountMap.get(key));
+ } catch (IOException e) {
+ LOG.error("Unable to write Container Key Prefix data in Recon DB.",
+ e);
+ }
+ });
+
+ deletedContainerKeyList.forEach((ContainerKeyPrefix key) -> {
+ try {
+ reconContainerMetadataManager
+ .batchDeleteContainerMapping(rdbBatchOperation, key);
+ } catch (IOException e) {
+ LOG.error("Unable to write Container Key Prefix data in Recon DB.",
+ e);
+ }
+ });
+
+ reconContainerMetadataManager.commitBatchOperation(rdbBatchOperation);
+ }
+
/**
* Delete an OM Key from Container DB and update containerID -> no. of keys
* count.
*
* @param key key String.
Review Comment:
change the javadoc accordingly to show that this is _preparing for batch deletion (IIUC)_. and list the new params. same for others.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/NSSummaryTask.java:
##########
@@ -227,38 +263,46 @@ private void writeOmKeyInfoOnNamespaceDB(OmKeyInfo keyInfo)
++fileBucket[binIndex];
nsSummary.setFileSizeBucket(fileBucket);
- reconNamespaceSummaryManager.storeNSSummary(parentObjectId, nsSummary);
+ nsSummaryMap.put(parentObjectId, nsSummary);
}
- private void writeOmDirectoryInfoOnNamespaceDB(OmDirectoryInfo directoryInfo)
+ private void handlePutDirEvent(OmDirectoryInfo directoryInfo,
+ Map<Long, NSSummary> nsSummaryMap)
throws IOException {
long parentObjectId = directoryInfo.getParentObjectID();
long objectId = directoryInfo.getObjectID();
// write the dir name to the current directory
String dirName = directoryInfo.getName();
- NSSummary curNSSummary =
- reconNamespaceSummaryManager.getNSSummary(objectId);
+ NSSummary curNSSummary = nsSummaryMap.get(objectId);
+ if (curNSSummary == null) {
+ curNSSummary = reconNamespaceSummaryManager.getNSSummary(objectId);
+ }
if (curNSSummary == null) {
curNSSummary = new NSSummary();
}
curNSSummary.setDirName(dirName);
- reconNamespaceSummaryManager.storeNSSummary(objectId, curNSSummary);
+ nsSummaryMap.put(objectId, curNSSummary);
// write the child dir list to the parent directory
- NSSummary nsSummary = reconNamespaceSummaryManager
- .getNSSummary(parentObjectId);
+ NSSummary nsSummary = nsSummaryMap.get(parentObjectId);
+ if (nsSummary == null) {
+ nsSummary = reconNamespaceSummaryManager.getNSSummary(parentObjectId);
+ }
if (nsSummary == null) {
nsSummary = new NSSummary();
}
nsSummary.addChildDir(objectId);
- reconNamespaceSummaryManager.storeNSSummary(parentObjectId, nsSummary);
+ nsSummaryMap.put(parentObjectId, nsSummary);
}
- private void deleteOmKeyInfoOnNamespaceDB(OmKeyInfo keyInfo)
+ private void handleDeleteKeyEvent(OmKeyInfo keyInfo,
+ Map<Long, NSSummary> nsSummaryMap)
throws IOException {
long parentObjectId = keyInfo.getParentObjectID();
- NSSummary nsSummary = reconNamespaceSummaryManager
- .getNSSummary(parentObjectId);
+ NSSummary nsSummary = nsSummaryMap.get(parentObjectId);
+ if (nsSummary == null) {
+ nsSummary = reconNamespaceSummaryManager.getNSSummary(parentObjectId);
Review Comment:
Same here
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ContainerKeyMapperTask.java:
##########
@@ -220,27 +311,34 @@ private void writeOMKeyToContainerDB(String key, OmKeyInfo omKeyInfo)
ContainerKeyPrefix containerKeyPrefix = new ContainerKeyPrefix(
containerId, key, keyVersion);
if (reconContainerMetadataManager.getCountForContainerKeyPrefix(
- containerKeyPrefix) == 0) {
+ containerKeyPrefix) == 0
+ && !containerKeyMap.containsKey(containerKeyPrefix)) {
// Save on writes. No need to save same container-key prefix
// mapping again.
- reconContainerMetadataManager.storeContainerKeyMapping(
- containerKeyPrefix, 1);
+ containerKeyMap.put(containerKeyPrefix, 1);
Review Comment:
Will the value of each entry in `containerKeyMap` always be `1`? Will there be other use cases in the future?
Maybe I didn't fully understand this, but if it is just for checking the key existence, wouldn't a `Set` be more memory-efficient?
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ContainerKeyMapperTask.java:
##########
@@ -70,6 +74,10 @@ public ContainerKeyMapperTask(ReconContainerMetadataManager
@Override
public Pair<String, Boolean> reprocess(OMMetadataManager omMetadataManager) {
long omKeyCount = 0;
+ Map<ContainerKeyPrefix, Integer> containerKeyMap = new HashMap<>();
+ Map<Long, Long> containerKeyCountMap = new HashMap<>();
+ List<ContainerKeyPrefix> deletedKeyCountList = new ArrayList<>();
Review Comment:
Let's add a brief comment above each of those variables to explain how they are used (what do they store, e.g. maps from what key to what value, and how the rest of the process interacts with them).
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ReconNamespaceSummaryManagerImpl.java:
##########
@@ -66,6 +75,11 @@ public NSSummary getNSSummary(long objectId) throws IOException {
return nsSummaryTable.get(objectId);
}
+ public void commitBatchOperation(RDBBatchOperation rdbBatchOperation)
Review Comment:
nit: missing `@Override`.
The `@Override` annotation is here optional but it is a good practice to add it to prevent simple mistakes. See https://stackoverflow.com/a/212624
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/NSSummaryTask.java:
##########
@@ -205,15 +218,38 @@ public Pair<String, Boolean> reprocess(OMMetadataManager omMetadataManager) {
return new ImmutablePair<>(getTaskName(), false);
}
+ try {
+ writeNSSummariesToDB(nsSummaryMap);
+ } catch (IOException e) {
+ LOG.error("Unable to write Namespace Summary data in Recon DB.", e);
+ return new ImmutablePair<>(getTaskName(), false);
+ }
LOG.info("Completed a reprocess run of NSSummaryTask");
return new ImmutablePair<>(getTaskName(), true);
}
- private void writeOmKeyInfoOnNamespaceDB(OmKeyInfo keyInfo)
- throws IOException {
+ private void writeNSSummariesToDB(Map<Long, NSSummary> nsSummaryMap)
+ throws IOException {
+ RDBBatchOperation rdbBatchOperation = new RDBBatchOperation();
+ nsSummaryMap.keySet().forEach((Long key) -> {
+ try {
+ reconNamespaceSummaryManager.batchStoreNSSummaries(rdbBatchOperation,
+ key, nsSummaryMap.get(key));
+ } catch (IOException e) {
+ LOG.error("Unable to write Namespace Summary data in Recon DB.",
+ e);
+ }
+ });
+ reconNamespaceSummaryManager.commitBatchOperation(rdbBatchOperation);
+ }
+
+ private void handlePutKeyEvent(OmKeyInfo keyInfo, Map<Long,
+ NSSummary> nsSummaryMap) throws IOException {
long parentObjectId = keyInfo.getParentObjectID();
- NSSummary nsSummary = reconNamespaceSummaryManager
- .getNSSummary(parentObjectId);
+ NSSummary nsSummary = nsSummaryMap.get(parentObjectId);
+ if (nsSummary == null) {
+ nsSummary = reconNamespaceSummaryManager.getNSSummary(parentObjectId);
Review Comment:
If `nsSummaryMap.get()` is not expected to return null, we can add a `LOG.warn` or `LOG.error` here. If `nsSummaryMap.get()` could return null, we can add a `LOG.debug` message instead.
Maybe adding some comments would also help others understand when would `nsSummary` be null in the first place.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/NSSummaryTask.java:
##########
@@ -279,15 +323,18 @@ private void deleteOmKeyInfoOnNamespaceDB(OmKeyInfo keyInfo)
nsSummary.setSizeOfFiles(sizeOfFile - dataSize);
--fileBucket[binIndex];
nsSummary.setFileSizeBucket(fileBucket);
- reconNamespaceSummaryManager.storeNSSummary(parentObjectId, nsSummary);
+ nsSummaryMap.put(parentObjectId, nsSummary);
}
- private void deleteOmDirectoryInfoOnNamespaceDB(OmDirectoryInfo directoryInfo)
+ private void handleDeleteDirEvent(OmDirectoryInfo directoryInfo,
+ Map<Long, NSSummary> nsSummaryMap)
throws IOException {
long parentObjectId = directoryInfo.getParentObjectID();
long objectId = directoryInfo.getObjectID();
- NSSummary nsSummary = reconNamespaceSummaryManager
- .getNSSummary(parentObjectId);
+ NSSummary nsSummary = nsSummaryMap.get(parentObjectId);
+ if (nsSummary == null) {
+ nsSummary = reconNamespaceSummaryManager.getNSSummary(parentObjectId);
Review Comment:
Same here
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/NSSummaryTask.java:
##########
@@ -227,38 +263,46 @@ private void writeOmKeyInfoOnNamespaceDB(OmKeyInfo keyInfo)
++fileBucket[binIndex];
nsSummary.setFileSizeBucket(fileBucket);
- reconNamespaceSummaryManager.storeNSSummary(parentObjectId, nsSummary);
+ nsSummaryMap.put(parentObjectId, nsSummary);
}
- private void writeOmDirectoryInfoOnNamespaceDB(OmDirectoryInfo directoryInfo)
+ private void handlePutDirEvent(OmDirectoryInfo directoryInfo,
+ Map<Long, NSSummary> nsSummaryMap)
throws IOException {
long parentObjectId = directoryInfo.getParentObjectID();
long objectId = directoryInfo.getObjectID();
// write the dir name to the current directory
String dirName = directoryInfo.getName();
- NSSummary curNSSummary =
- reconNamespaceSummaryManager.getNSSummary(objectId);
+ NSSummary curNSSummary = nsSummaryMap.get(objectId);
+ if (curNSSummary == null) {
+ curNSSummary = reconNamespaceSummaryManager.getNSSummary(objectId);
Review Comment:
Same here
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/TableCountTask.java:
##########
@@ -69,18 +75,18 @@ public TableCountTask(GlobalStatsDao globalStatsDao,
*/
@Override
public Pair<String, Boolean> reprocess(OMMetadataManager omMetadataManager) {
+ HashMap<String, Long> objectCountMap = initializeCountMap();
for (String tableName : getTaskTables()) {
Table table = omMetadataManager.getTable(tableName);
try (TableIterator keyIter = table.iterator()) {
long count = getCount(keyIter);
- ReconUtils.upsertGlobalStatsTable(sqlConfiguration, globalStatsDao,
Review Comment:
You mentioned:
> With that I didn't find upsert query or anything similar, so I batched the updates and insert separately. This could be improved in the future, but for now this to cause a problem is an event with a small probability of occurrence.
Do you mean the `upsert` here? Would you elaborate on what problem could this raise when we switch to `map.put`?
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/NSSummaryTask.java:
##########
@@ -205,15 +218,38 @@ public Pair<String, Boolean> reprocess(OMMetadataManager omMetadataManager) {
return new ImmutablePair<>(getTaskName(), false);
}
+ try {
+ writeNSSummariesToDB(nsSummaryMap);
+ } catch (IOException e) {
+ LOG.error("Unable to write Namespace Summary data in Recon DB.", e);
+ return new ImmutablePair<>(getTaskName(), false);
+ }
LOG.info("Completed a reprocess run of NSSummaryTask");
return new ImmutablePair<>(getTaskName(), true);
}
- private void writeOmKeyInfoOnNamespaceDB(OmKeyInfo keyInfo)
- throws IOException {
+ private void writeNSSummariesToDB(Map<Long, NSSummary> nsSummaryMap)
+ throws IOException {
+ RDBBatchOperation rdbBatchOperation = new RDBBatchOperation();
+ nsSummaryMap.keySet().forEach((Long key) -> {
+ try {
+ reconNamespaceSummaryManager.batchStoreNSSummaries(rdbBatchOperation,
+ key, nsSummaryMap.get(key));
+ } catch (IOException e) {
+ LOG.error("Unable to write Namespace Summary data in Recon DB.",
+ e);
+ }
+ });
+ reconNamespaceSummaryManager.commitBatchOperation(rdbBatchOperation);
+ }
+
+ private void handlePutKeyEvent(OmKeyInfo keyInfo, Map<Long,
+ NSSummary> nsSummaryMap) throws IOException {
long parentObjectId = keyInfo.getParentObjectID();
- NSSummary nsSummary = reconNamespaceSummaryManager
- .getNSSummary(parentObjectId);
+ NSSummary nsSummary = nsSummaryMap.get(parentObjectId);
+ if (nsSummary == null) {
+ nsSummary = reconNamespaceSummaryManager.getNSSummary(parentObjectId);
+ }
if (nsSummary == null) {
nsSummary = new NSSummary();
Review Comment:
Same around here.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/NSSummaryTask.java:
##########
@@ -227,38 +263,46 @@ private void writeOmKeyInfoOnNamespaceDB(OmKeyInfo keyInfo)
++fileBucket[binIndex];
nsSummary.setFileSizeBucket(fileBucket);
- reconNamespaceSummaryManager.storeNSSummary(parentObjectId, nsSummary);
+ nsSummaryMap.put(parentObjectId, nsSummary);
}
- private void writeOmDirectoryInfoOnNamespaceDB(OmDirectoryInfo directoryInfo)
+ private void handlePutDirEvent(OmDirectoryInfo directoryInfo,
+ Map<Long, NSSummary> nsSummaryMap)
throws IOException {
long parentObjectId = directoryInfo.getParentObjectID();
long objectId = directoryInfo.getObjectID();
// write the dir name to the current directory
String dirName = directoryInfo.getName();
- NSSummary curNSSummary =
- reconNamespaceSummaryManager.getNSSummary(objectId);
+ NSSummary curNSSummary = nsSummaryMap.get(objectId);
+ if (curNSSummary == null) {
+ curNSSummary = reconNamespaceSummaryManager.getNSSummary(objectId);
+ }
if (curNSSummary == null) {
curNSSummary = new NSSummary();
}
curNSSummary.setDirName(dirName);
- reconNamespaceSummaryManager.storeNSSummary(objectId, curNSSummary);
+ nsSummaryMap.put(objectId, curNSSummary);
// write the child dir list to the parent directory
- NSSummary nsSummary = reconNamespaceSummaryManager
- .getNSSummary(parentObjectId);
+ NSSummary nsSummary = nsSummaryMap.get(parentObjectId);
+ if (nsSummary == null) {
+ nsSummary = reconNamespaceSummaryManager.getNSSummary(parentObjectId);
Review Comment:
Same here
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org