ivandika3 commented on code in PR #10946:
URL: https://github.com/apache/ozone/pull/10946#discussion_r3771702628
##########
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;
+
/**
* Builder for AllocatedBlock.
*/
public static class Builder {
private Pipeline pipeline;
private ContainerBlockID containerBlockID;
+ private StorageTier storageTier;
Review Comment:
Nit: `@Nullable` here.
##########
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;
Review Comment:
Nit: `@Nullable` here.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java:
##########
@@ -171,28 +172,48 @@ public AllocatedBlock allocateBlock(final long size,
// configured, it will be of type StorageType.DISK and therefore belong
// to a StorageTier.DISK tier, so for old clients the write process is
// unchanged if the Datanode Volume configuration is not changed.
- StorageTier storageTier = StorageTier.getDefaultTier();
- ContainerInfo containerInfo = writableContainerFactory.getContainer(
- size, replicationConfig, owner, excludeList, storageTier);
+ boolean isFallBack = false;
+ ContainerInfo containerInfo = null;
+ try {
+ containerInfo = writableContainerFactory.getContainer(
+ size, replicationConfig, owner, excludeList,
storagePolicy.getCreationTier());
+ } catch (IOException e) {
+ if (allowFallbackStoragePolicy &&
storagePolicy.getCreationFallbackTier() != StorageTier.EMPTY) {
+ // TODO StoragePolicy should It should be distinguished in detail
which exceptions can try to fallback
+ isFallBack = true;
+ containerInfo = writableContainerFactory.getContainer(size,
replicationConfig, owner,
+ excludeList, storagePolicy.getCreationFallbackTier());
+ } else {
+ throw e;
+ }
+ }
+ if (containerInfo == null && allowFallbackStoragePolicy) {
+ isFallBack = true;
+ containerInfo = writableContainerFactory.getContainer(size,
replicationConfig, owner,
+ excludeList, storagePolicy.getCreationFallbackTier());
+ }
Review Comment:
We already have a fallback logic in the catch block, also internally we
don't have this logic.
Won't this cause a worst case scenario of calling fallback 2 times?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/ScmBlockLocationProtocolServerSideTranslatorPB.java:
##########
@@ -193,6 +194,12 @@ private Status exceptionToResponseStatus(IOException ex) {
public AllocateScmBlockResponseProto allocateScmBlock(
AllocateScmBlockRequestProto request, int clientVersion)
throws IOException {
+ OzoneStoragePolicy storagePolicy;
+ if (request.hasStoragePolicy()) {
+ storagePolicy = OzoneStoragePolicy.fromProto(request.getStoragePolicy());
+ } else {
+ storagePolicy = OzoneStoragePolicy.getDefaultPolicy();
+ }
Review Comment:
Please help to validate discrepancy with the internal logic.
```java
if (request.hasStoragePolicy()) {
storagePolicy =
OzoneStoragePolicy.fromProto(request.getStoragePolicy());
// If the storagePolicy was specific, then the field allowFallBack
must be specified.
Preconditions.checkArgument(request.hasAllowFallBack());
allowFallback = request.getAllowFallBack();
} else {
// When the request comes from an old OM that does not support
StoragePolicy,
// StoragePolicy will not be explicitly set. The default StoragePolicy
is used here.
storagePolicy = OzoneStoragePolicy.getDefaultPolicy();
allowFallback = false;
}
```
We have the `optional bool allowFallBack = 11 [default = true]`, please
check what happens when an old client sends a request, will `allowFallBack` be
true or false? Is this expected?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java:
##########
@@ -171,28 +172,48 @@ public AllocatedBlock allocateBlock(final long size,
// configured, it will be of type StorageType.DISK and therefore belong
// to a StorageTier.DISK tier, so for old clients the write process is
// unchanged if the Datanode Volume configuration is not changed.
- StorageTier storageTier = StorageTier.getDefaultTier();
- ContainerInfo containerInfo = writableContainerFactory.getContainer(
- size, replicationConfig, owner, excludeList, storageTier);
+ boolean isFallBack = false;
+ ContainerInfo containerInfo = null;
+ try {
+ containerInfo = writableContainerFactory.getContainer(
+ size, replicationConfig, owner, excludeList,
storagePolicy.getCreationTier());
+ } catch (IOException e) {
+ if (allowFallbackStoragePolicy &&
storagePolicy.getCreationFallbackTier() != StorageTier.EMPTY) {
+ // TODO StoragePolicy should It should be distinguished in detail
which exceptions can try to fallback
+ isFallBack = true;
+ containerInfo = writableContainerFactory.getContainer(size,
replicationConfig, owner,
+ excludeList, storagePolicy.getCreationFallbackTier());
+ } else {
+ throw e;
+ }
Review Comment:
Should we throw the exception here? Internally, we don't throw exception
here.
I just reviewed BlockManagerImpl#allocateBlock logic. It seems currently we
will allow letting the `allocateBlock` to return null, but we simply skip this
block. Meaning if client asks for 2 blocks, but only 1 is provided, then we
will return only 1 block instead of failing the whole request. Is this expected
behavior? We might need to revisit this.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockLocationInfo.java:
##########
@@ -42,6 +43,8 @@ public class BlockLocationInfo {
private int partNumber;
// The block is under construction. Apply to hsynced file last block.
private boolean underConstruction;
+ private StorageTier storageTier;
Review Comment:
Nit: `Nullable` 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: [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]
