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 2021/03/22 12:45:16 UTC
[GitHub] [ozone] elek opened a new pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
elek opened a new pull request #2069:
URL: https://github.com/apache/ozone/pull/2069
## What changes were proposed in this pull request?
When a container is created for EC replication, the `replicaIndex` metadata should be persisted to the container yaml file and reported back to the SCM with the container reports.
Note: this PR requires #2055 merged (therefore I mark it as draft, but feel free to comment it)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4883
## How was this patch tested?
e2e tests with https://github.com/elek/ozone/tree/ec
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r608441478
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
Very good question, I planned to discuss it.
```
- // This test is for if we upgrade, and then .container files added by new
- // server will have new fields added to .container file, after a while we
- // decided to rollback. Then older ozone can read .container files
- // created or not.
```
This is a limitation of the current checksum calculation. With this restriction we can never add new fields to the container file.
I think the proper fix here is removing the test and using the upgrade framework to avoid some situation. This unit test is perfect for the master, temporary, but we should have an option to add new fields if new features are allowed (after finalize).
Today it's not possible as upgrade is not merged, but will be possible with upgrade framework (or just denying all the new EC requests). Therefore, I think it's safe to remove the unit test, move forward, and later add specific upgrade tests for EC.
(cc @avijayanhwx )
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r633462857
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
> Anyway for the EC containers, we need to deal. One question: do we allow old code DN to serve the EC container data?
No. It's not possible as EC data will be written only after finalization which means that old DN code (downgrade) won't be supported any more.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek merged pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek merged pull request #2069:
URL: https://github.com/apache/ozone/pull/2069
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r612246235
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
Yes, this is possible, but we wouldn't like to introduce new checksum fields when we introduce new fields. It should be done (IMHO) together with another fix which ensures that the new checksum field is always backward compatible (for example because all the fields are used to calculate the checksum).
I totally agree that it should be done, but I think it should be done in a separated issue / patch.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r638013481
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerDataYaml.java
##########
@@ -27,10 +27,7 @@
import java.io.OutputStreamWriter;
import java.io.Writer;
import java.nio.charset.StandardCharsets;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.TreeSet;
+import java.util.*;
Review comment:
nit: can we just avoid this auto organize?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r611776386
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
I understand your point.
I think we can try the following way to handle this? ( we discussed this point and to keep the discussion open, I am posting the comment here)
How about having additional field which calculates the new checksum including new field. Old checksum field unchanged, so, older clients will go with old checksum and newer clients will check new checksum field?
Let's see if this can work.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#issuecomment-847911029
Thanks for the review @umamaheswararao. Merging it now (import is fixed and we have green build)
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r613593892
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
I was thinking the newer client will always do checksums for available fields in file, but not from java enum fields. Older client will do as is.
Also think about the option ignore the additional field when replIndex is 0, which is default. So, for non-existent we can assume that as 0 in newer clients.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r608437838
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -410,6 +410,11 @@ ContainerCommandResponseProto createContainer(
ContainerProtos.ContainerType.KeyValueContainer;
createRequest.setContainerType(containerType);
+ if (containerRequest.hasWriteChunk()) {
Review comment:
Sorry, I am not sure about the question. What do you mean `pack from the BockID`?
I think you suggested including the replicaIndex in the block id which became a `(containerid, localid, replicaindex)` tuplet. This is the reason to have `getBlockd().getReplicaIndex()` here.
But let me know if I misunderstand something.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r631330772
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
I meant that, after EC branch merged, but not using EC containers at all, will have not any impact at all if we skip writing index.
Anyway for the EC containers, we need to deal. One question: do we allow old code DN to serve the EC container data?
I am worried that Old DN will not know if there is any EC specific logic added at DN.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r608007205
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -410,6 +410,11 @@ ContainerCommandResponseProto createContainer(
ContainerProtos.ContainerType.KeyValueContainer;
createRequest.setContainerType(containerType);
+ if (containerRequest.hasWriteChunk()) {
Review comment:
I think we decided to pack the index from pipeline object but not from BlockID right?
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
Why this test removed?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r633462857
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
> Anyway for the EC containers, we need to deal. One question: do we allow old code DN to serve the EC container data?
No. It's not possible as EC data will be written only after finalization which means that old DN code (downgrade) won't be supported any more.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r611777897
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -410,6 +410,11 @@ ContainerCommandResponseProto createContainer(
ContainerProtos.ContainerType.KeyValueContainer;
createRequest.setContainerType(containerType);
+ if (containerRequest.hasWriteChunk()) {
Review comment:
I got ur point. I confused because we have not added the code to send the index from client yet, but we are trying to using here. For completion it may be good to include client side sending this idx?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r612247832
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -410,6 +410,11 @@ ContainerCommandResponseProto createContainer(
ContainerProtos.ContainerType.KeyValueContainer;
createRequest.setContainerType(containerType);
+ if (containerRequest.hasWriteChunk()) {
Review comment:
A simplified client side change is here: https://github.com/elek/ozone/commit/c86878842f29f4c5f08a16a5959c51c0a275ee3b
I think this is a well scoped and unit-tested change, client side change may need additional work. But if you would like to include this small commit, I would be happy to add 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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#issuecomment-843932021
@umamaheswararao #4c243349b persists `replicaIndex` (and use it in checksum) **only if** it's >0. Somewhat hacky but works.
Can you PTAL?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r633694437
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
Yes. Let's skip the replicationIndex=0 field writing to yaml file in this patch.
@avijayanhwx If we bump keyvalueContainer version, does the upgrade framework handled to check the version numbers?
Currently, as you noticed in this JIRA, we need to write one additional field in yaml file for EC. Since it's changing the on disk metadata, we need to worry about compt. While in upgrade in progress, your plan is not to allow new features to be used right? In that case, if you are already having check for keyValue container version, then bumping version would make things cleaner. Could you please comment your thoughts here? Thanks
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r617471238
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
> Also think about the option ignore the additional field when replIndex is 0, which is default
We can do it, but it doesn't help the compatibility issue, and it requires a bigger refactor on the write code. I checked it, and it requires refactoring `ContainerDataRepresenter` (or the surrounding code) which may require bigger work.
As we need to take care about the compatibility issue related to EC calls anyway, I suggest using this patch as is to move forward, and we will take care about all the compatibility testing after the upgrade framework is merged.
Improving the checksum calculation of container files seems to be a good idea anyway, I created https://issues.apache.org/jira/browse/HDDS-5129 to track the issue (and discuss the best approach as there are multiple approach).
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r622558583
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
>We can do it, but it doesn't help the compatibility issue, and it requires a bigger refactor on the write code. I checked it, and it requires refactoring ContainerDataRepresenter (or the surrounding code) which may require bigger work.
With ignoring replIndex =0, for EC disabled clusters should not have any impact right?
Thanks for creating the separated JIRA for improving.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r622599284
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -410,6 +410,11 @@ ContainerCommandResponseProto createContainer(
ContainerProtos.ContainerType.KeyValueContainer;
createRequest.setContainerType(containerType);
+ if (containerRequest.hasWriteChunk()) {
Review comment:
It is fine for me. When need at client side we can add that.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#issuecomment-836283933
ping @umamaheswararao
Please let me know if you have any remaining concerns.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r625039024
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
> With ignoring replIndex =0, for EC disabled clusters should not have any impact right?
Sorry, I am not sure if I understood this question.
2. Today we have a generic code to serialize all the white-listed fields. It can be modified to support default values and write out spefic if it has a value different from the default, but it requires some code re-organization.
1. Even if we do this (do not write replIndex to the container file for non EC containers) it doesn't help the backward compatible problem. When a new EC type of container is written (eg. replIndex=1) it couldn't be read by the old cluster code (as it's not known if it's an EC container or not). Therefore, we should enable to write the replIndex only after the finalize step). In this case there is no advantage to write or not write `replIndex` to the container yaml file for normal containers. (It's more like a code style question, if you think it's worth to add more code re-organization for this, I am fine to add it, but from behavior point of view we will have the same results).
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #2069: HDDS-4883. Persist replicationIndex on datanode side
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2069:
URL: https://github.com/apache/ozone/pull/2069#discussion_r633694437
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java
##########
@@ -179,44 +180,7 @@ public void testIncorrectContainerFile() throws IOException{
GenericTestUtils.assertExceptionContains("No enum constant", ex);
}
}
-
-
Review comment:
Yes. Let's skip the replicationIndex=0 field writing to yaml file in this patch.
@avijayanhwx If we bump keyvalueContainer version, does the upgrade framework handled to check the version numbers?
Currently, as you noticed in this JIRA, we need to write one additional field in yaml file for EC. Since it's changing the on disk metadata, we need to worry about compt. While in upgrade in progress, your plan is not to allow new features to be used right? In that case, if you are already having check for keyValue container version, then bumping version would make things cleaner. Could you please comment your thoughts here? Thanks
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org