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 2020/04/30 07:57:46 UTC
[GitHub] [hadoop-ozone] sadanand48 opened a new pull request #886: HDDS-3473.ozone chunkinfo CLI should display block file path info
sadanand48 opened a new pull request #886:
URL: https://github.com/apache/hadoop-ozone/pull/886
## What changes were proposed in this pull request?
The debug tool CLI was not considering both the file per block and file per chunk strategies while displaying the file path .This change fixes the issue and depending on the strategy , the path is shown in a field called files.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3473
## How was this patch tested?
Manually tested.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] mukul1987 commented on a change in pull request #886: HDDS-3473.ozone chunkinfo CLI should display block file path info
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #886:
URL: https://github.com/apache/hadoop-ozone/pull/886#discussion_r417827508
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ChunkKeyHandler.java
##########
@@ -128,17 +131,23 @@ protected void execute(OzoneClient client, OzoneAddress address)
chunkDetails.setChunkName(chunkInfo.getChunkName());
chunkDetails.setChunkOffset(chunkInfo.getOffset());
chunkDetailsList.add(chunkDetails);
- chunkPaths.add(getChunkLocationPath(containerData.getContainerPath())
- + File.separator
- + chunkInfo.getChunkName());
+ if (chunkLayOutVersion == ChunkLayOutVersion.FILE_PER_CHUNK) {
+ chunkPaths.add(getChunkLocationPath(containerData.getContainerPath())
+ + File.separator
+ + chunkInfo.getChunkName());
+ } else {
Review comment:
lets convert this if into a switch case, in case new CHunkLayoutVersions are added later, there should be an option of adding it later. Also for default case lets throw an exception.
##########
File path: hadoop-ozone/dist/src/main/smoketest/basic/ozone-shell.robot
##########
@@ -121,7 +121,7 @@ Test key handling
${result} = Execute ozone sh key info ${protocol}${server}/${volume}/bb1/key1 | jq -r '. | select(.name=="key1")'
Should contain ${result} creationTime
${result} = Execute ozone debug chunkinfo ${protocol}${server}/${volume}/bb1/key1 | jq -r '.[]'
- Should contain ${result} chunks
+ Should contain ${result} files
Review comment:
Can this command be executed on the datanode container and lets do a check that the file exists.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bshashikant commented on a change in pull request #886: HDDS-3473. Ozone chunkinfo CLI should display block file path info
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #886:
URL: https://github.com/apache/hadoop-ozone/pull/886#discussion_r419880045
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ChunkKeyHandler.java
##########
@@ -128,17 +132,31 @@ protected void execute(OzoneClient client, OzoneAddress address)
chunkDetails.setChunkName(chunkInfo.getChunkName());
chunkDetails.setChunkOffset(chunkInfo.getOffset());
chunkDetailsList.add(chunkDetails);
- chunkPaths.add(getChunkLocationPath(containerData.getContainerPath())
- + File.separator
- + chunkInfo.getChunkName());
+ switch (chunkLayOutVersion) {
+ case FILE_PER_CHUNK:
+ chunkPaths.add(getChunkLocationPath(containerData
+ .getContainerPath())
+ + File.separator
+ + chunkInfo.getChunkName());
+ break;
+ case FILE_PER_BLOCK:
+ chunkPaths.add(getChunkLocationPath(containerData
+ .getContainerPath())
+ + File.separator
+ + keyLocation.getLocalID() + ".block");
+ break;
+ default:
+ throw new StorageContainerException("chunk strategy does not exist",
+ ContainerProtos.Result.UNABLE_TO_FIND_CHUNK);
+ }
Review comment:
I think we should move the code to give the chunk file path given the containerID/ContainerData into some common utilities class and use it across client/server so that any further changes won't break it..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sadanand48 commented on a change in pull request #886: HDDS-3473. Ozone chunkinfo CLI should display block file path info
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on a change in pull request #886:
URL: https://github.com/apache/hadoop-ozone/pull/886#discussion_r420015257
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ChunkFileUtility.java
##########
@@ -0,0 +1,39 @@
+package org.apache.hadoop.ozone.debug;
+
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.impl.ChunkLayOutVersion;
+import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
+
+import java.io.File;
+
+
+public final class ChunkFileUtility {
+
+ static String getChunkLocationPath(String containerLocation) {
+ return containerLocation + File.separator + OzoneConsts.STORAGE_DIR_CHUNKS;
+ }
+
+ public static String getChunkFilePath(ContainerProtos.ChunkInfo
+ chunkInfo, OmKeyLocationInfo keyLocation,
+ ContainerProtos.ContainerDataProto data,
+ ChunkLayOutVersion layOutVersion)
+ throws StorageContainerException {
+ switch (layOutVersion) {
+ case FILE_PER_CHUNK:
+ return getChunkLocationPath(data
+ .getContainerPath())
+ + File.separator
+ + chunkInfo.getChunkName();
+ case FILE_PER_BLOCK:
+ return getChunkLocationPath(data
+ .getContainerPath())
+ + File.separator
+ + keyLocation.getLocalID() + ".block";
+ default:
+ throw new StorageContainerException("chunk strategy does not exist",
+ ContainerProtos.Result.UNABLE_TO_FIND_CHUNK);
+ }
Review comment:
Thankyou @adoroszlai for the review. Addressed review comments .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bshashikant merged pull request #886: HDDS-3473. Ozone chunkinfo CLI should display block file path info
Posted by GitBox <gi...@apache.org>.
bshashikant merged pull request #886:
URL: https://github.com/apache/hadoop-ozone/pull/886
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sadanand48 commented on a change in pull request #886: HDDS-3473. Ozone chunkinfo CLI should display block file path info
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on a change in pull request #886:
URL: https://github.com/apache/hadoop-ozone/pull/886#discussion_r419460129
##########
File path: hadoop-ozone/dist/src/main/smoketest/basic/ozone-shell.robot
##########
@@ -121,7 +121,7 @@ Test key handling
${result} = Execute ozone sh key info ${protocol}${server}/${volume}/bb1/key1 | jq -r '. | select(.name=="key1")'
Should contain ${result} creationTime
${result} = Execute ozone debug chunkinfo ${protocol}${server}/${volume}/bb1/key1 | jq -r '.[]'
- Should contain ${result} chunks
+ Should contain ${result} files
Review comment:
I have included a separate robot test that checks whether the file is present in the datanode.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #886: HDDS-3473. Ozone chunkinfo CLI should display block file path info
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #886:
URL: https://github.com/apache/hadoop-ozone/pull/886#discussion_r421289937
##########
File path: hadoop-ozone/dist/src/main/smoketest/debug/ozone-debug.robot
##########
@@ -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.
+
+*** Settings ***
+Documentation Test ozone Debug CLI
+Library OperatingSystem
+Resource ../commonlib.robot
+Test Timeout 2 minute
+
+*** Variables ***
+
+
+*** Test Cases ***
+Create Volume,Bucket and put key
+ Execute ozone sh volume create o3://om/vol1 --quota 100TB
+ Execute ozone sh bucket create o3://om/vol1/bucket1
+ Execute ozone sh key put o3://om/vol1/bucket1/debugKey /opt/hadoop/NOTICE.txt
+
+Test ozone debug
+ ${result} = Execute ozone debug chunkinfo o3://om/vol1/bucket1/debugKey | jq -r '.[]'
+ Should contain ${result} files
+ ${result} = Execute ozone debug chunkinfo o3://om/vol1/bucket1/debugKey | jq -r '.[].files[0]'
+ ${result3} = Execute echo "exists"
+ ${result2} = Execute test -f ${result} && echo "exists"
+ Should Be Equal ${result2} ${result3}
Review comment:
`Execute` checks the return code of the command, so I think this should be enough to verify that the file exists:
```
Execute test -f ${result}
```
Better yet, there is a Robot keyword for the same: [`File Should Exist`](http://robotframework.org/robotframework/3.0.2/libraries/OperatingSystem.html#File%20Should%20Exist)
```suggestion
File Should Exist ${result}
```
##########
File path: hadoop-ozone/dist/src/main/smoketest/debug/ozone-debug.robot
##########
@@ -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.
+
+*** Settings ***
+Documentation Test ozone Debug CLI
+Library OperatingSystem
+Resource ../commonlib.robot
+Test Timeout 2 minute
+
+*** Variables ***
+
+
+*** Test Cases ***
+Create Volume,Bucket and put key
+ Execute ozone sh volume create o3://om/vol1 --quota 100TB
+ Execute ozone sh bucket create o3://om/vol1/bucket1
+ Execute ozone sh key put o3://om/vol1/bucket1/debugKey /opt/hadoop/NOTICE.txt
Review comment:
I think this should be a `Suite Setup` instead of a `Test Case`, but it's OK to improve in a followup Jira.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #886: HDDS-3473. Ozone chunkinfo CLI should display block file path info
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #886:
URL: https://github.com/apache/hadoop-ozone/pull/886#discussion_r419915270
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ChunkFileUtility.java
##########
@@ -0,0 +1,39 @@
+package org.apache.hadoop.ozone.debug;
+
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.impl.ChunkLayOutVersion;
+import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
+
+import java.io.File;
+
+
+public final class ChunkFileUtility {
+
+ static String getChunkLocationPath(String containerLocation) {
+ return containerLocation + File.separator + OzoneConsts.STORAGE_DIR_CHUNKS;
+ }
+
+ public static String getChunkFilePath(ContainerProtos.ChunkInfo
+ chunkInfo, OmKeyLocationInfo keyLocation,
+ ContainerProtos.ContainerDataProto data,
+ ChunkLayOutVersion layOutVersion)
+ throws StorageContainerException {
+ switch (layOutVersion) {
+ case FILE_PER_CHUNK:
+ return getChunkLocationPath(data
+ .getContainerPath())
+ + File.separator
+ + chunkInfo.getChunkName();
+ case FILE_PER_BLOCK:
+ return getChunkLocationPath(data
+ .getContainerPath())
+ + File.separator
+ + keyLocation.getLocalID() + ".block";
+ default:
+ throw new StorageContainerException("chunk strategy does not exist",
+ ContainerProtos.Result.UNABLE_TO_FIND_CHUNK);
+ }
Review comment:
I'd prefer taking chunk file path directly from `ChunkLayOutVersion.getChunkFile()` instead of the `switch`. This way if a new enum constant is introduced, this code will work without any changes.
This needs a minor refactoring in `ChunkLayOutVersion`, which accepts a `ContainerData` object, to work directly with the chunk dir:
```diff
--- hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ChunkLayOutVersion.java
+++ hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ChunkLayOutVersion.java
@@ -40,17 +40,15 @@
FILE_PER_CHUNK(1, "One file per chunk") {
@Override
- public File getChunkFile(ContainerData containerData, BlockID blockID,
- ChunkInfo info) throws StorageContainerException {
- File chunksLoc = verifyChunkDirExists(containerData);
- return chunksLoc.toPath().resolve(info.getChunkName()).toFile();
+ public File getChunkFile(File chunkDir, BlockID blockID,
+ ChunkInfo info) {
+ return chunkDir.toPath().resolve(info.getChunkName()).toFile();
}
},
FILE_PER_BLOCK(2, "One file per block") {
@Override
- public File getChunkFile(ContainerData containerData, BlockID blockID,
- ChunkInfo info) throws StorageContainerException {
- File chunkDir = verifyChunkDirExists(containerData);
+ public File getChunkFile(File chunkDir, BlockID blockID,
+ ChunkInfo info) {
return new File(chunkDir, blockID.getLocalID() + ".block");
}
};
@@ -118,8 +116,14 @@ public String getDescription() {
return description;
}
- public abstract File getChunkFile(ContainerData containerData,
- BlockID blockID, ChunkInfo info) throws StorageContainerException;
+ public abstract File getChunkFile(File chunksDir,
+ BlockID blockID, ChunkInfo info);
+
+ public File getChunkFile(ContainerData containerData,
+ BlockID blockID, ChunkInfo info) throws StorageContainerException {
+ File chunksLoc = verifyChunkDirExists(containerData);
+ return getChunkFile(chunksLoc, blockID, info);
+ }
@Override
public String toString() {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org