greenwich commented on code in PR #10946:
URL: https://github.com/apache/ozone/pull/10946#discussion_r3763392192
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -729,6 +733,10 @@ private OzoneManager(OzoneConfiguration conf,
StartupOption startupOption)
omClientProtocolMetrics = ProtocolMessageMetrics
.create("OmClientProtocol", "Ozone Manager RPC endpoint",
OzoneManagerProtocolProtos.Type.class);
+ String configuredStoragePolicy = conf.get(
+ OZONE_DEFAULT_STORAGE_POLICY_KEY,
OZONE_DEFAULT_STORAGE_POLICY_DEFAULT);
+ OzoneStoragePolicy.setDefaultPolicy(OzoneStoragePolicy.valueOf(
Review Comment:
1. This line, in the OM constructor — the only call to setDefaultPolicy.
2. Reader: `SCM.
ScmBlockLocationProtocolServerSideTranslatorPB#allocateScmBlock` falls back to
`getDefaultPolicy()` when the incoming `AllocateScmBlockRequestProto` has no
`storagePolicy` set.
3. Will that work if OM and SCM are the separate JVMs? SCM always sees the
hardcoded `WARM`, and o`zone.default.storagepolicy` has no effect
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/common/helpers/AllocatedBlock.java:
##########
@@ -28,12 +29,17 @@ public final class AllocatedBlock {
private final Pipeline pipeline;
private final ContainerBlockID containerBlockID;
+ private StorageTier storageTier;
+ private boolean isFallBack;
Review Comment:
Is `isFallback` write-only? How do we currently use it, or how are we
planning to use it? I wasn’t able to trace any readers.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockLocationInfo.java:
##########
@@ -213,12 +249,14 @@ public boolean equals(Object o) {
createVersion == that.createVersion &&
Objects.equals(blockID, that.blockID) &&
Objects.equals(token, that.token) &&
- Objects.equals(pipeline, that.pipeline);
+ Objects.equals(pipeline, that.pipeline) &&
+ Objects.equals(storageTier, that.storageTier) &&
+ Objects.equals(isFallBack, that.isFallBack);
Review Comment:
It's related to my previous question about`isFallBack`..
Should an allocation-time hint decide whether two block locations are the
same block? For example, it makes a location decoded from a pre-upgrade record
never equal the same location after a fallback re-allocation.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMBlockProtocolServer.java:
##########
@@ -197,6 +200,8 @@ public List<AllocatedBlock> allocateBlock(
auditMap.put("replication", replicationConfig.toString());
auditMap.put("owner", owner);
auditMap.put("client", clientMachine);
+ auditMap.put("storagePolicy", storagePolicy.toString());
+ auditMap.put("allowFallbackStoragePolicy",
String.valueOf(allowFallbackStoragePolicy));
Review Comment:
Great that we have it in the audits but nothing records what happened —
`block.isFallBack()` is available in the loop at :229 and never makes it into
the audit map or a counter. With `allowFallBack` defaulting to `true`, a HOT
bucket can be served entirely from DISK with no metric, log, or audit entry
saying so.
Could we add the fallback count to the audit map and a counter on
`perfMetrics` alongside `updateAllocateBlockSuccessLatencyNs`?
--
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]
