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/05/16 05:36:23 UTC
[GitHub] [ozone] ChenSammi opened a new pull request, #3420: HDDS-6747. Support configurable separator between container ID and me…
ChenSammi opened a new pull request, #3420:
URL: https://github.com/apache/ozone/pull/3420
https://issues.apache.org/jira/browse/HDDS-6747
--
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
[GitHub] [ozone] guihecheng commented on pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
guihecheng commented on PR #3420:
URL: https://github.com/apache/ozone/pull/3420#issuecomment-1131168624
Thanks @ChenSammi , overall looks great, some small 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.
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3420:
URL: https://github.com/apache/ozone/pull/3420#discussion_r876715417
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerData.java:
##########
@@ -365,7 +365,7 @@ public String deletingBlockKeyPrefix() {
public KeyPrefixFilter getUnprefixedKeyFilter() {
String schemaPrefix = containerPrefix();
return new KeyPrefixFilter().addFilter(
- schemaPrefix == null ? "#" : schemaPrefix + "#", true);
Review Comment:
Good point.
--
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
[GitHub] [ozone] guihecheng commented on a diff in pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
guihecheng commented on code in PR #3420:
URL: https://github.com/apache/ozone/pull/3420#discussion_r876736262
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java:
##########
@@ -137,11 +143,16 @@ public DBColumnFamilyDefinition<String, Long> getMetadataColumnFamily() {
public static String getContainerKeyPrefix(long containerID) {
// NOTE: Rocksdb normally needs a fixed length prefix.
- return FixedLengthStringUtils.bytes2String(Longs.toByteArray(containerID));
+ return FixedLengthStringUtils.bytes2String(Longs.toByteArray(containerID))
Review Comment:
Ah, got it. I think if we follow the old way, the separator will be a part of the actual key, for example there is a blockKey, then this separator goes to the head of a blockKey(which is a localID), then things got messed up.
With the new approach, the separator is part of the prefix, even we got 1 byte more to compare, it is fine.
I think the ideal effect in my mind is that, the separator should below to neither the prefix nor the actual key.
Then, even we got the separator changed, we are still likely to parse the keys, for example:
change from '|' -> '#', if we parse the key like:
1. seek with 8bytes prefix
2. skip 1 byte for separator
3. parse the left bytes for blockKey.
Then we could make the separator isolated and independent, of course if you have a separator with different length, then it will not be possible.
So let's just go with this approach and see if we could do better in the future.
--
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3420:
URL: https://github.com/apache/ozone/pull/3420#discussion_r876719144
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java:
##########
@@ -137,11 +143,16 @@ public DBColumnFamilyDefinition<String, Long> getMetadataColumnFamily() {
public static String getContainerKeyPrefix(long containerID) {
// NOTE: Rocksdb normally needs a fixed length prefix.
- return FixedLengthStringUtils.bytes2String(Longs.toByteArray(containerID));
+ return FixedLengthStringUtils.bytes2String(Longs.toByteArray(containerID))
Review Comment:
Yes, some unit test failed. Forget which one. After moved the separator to this new place, it works.
--
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
[GitHub] [ozone] ChenSammi merged pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
ChenSammi merged PR #3420:
URL: https://github.com/apache/ozone/pull/3420
--
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
[GitHub] [ozone] guihecheng commented on a diff in pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
guihecheng commented on code in PR #3420:
URL: https://github.com/apache/ozone/pull/3420#discussion_r876736262
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java:
##########
@@ -137,11 +143,16 @@ public DBColumnFamilyDefinition<String, Long> getMetadataColumnFamily() {
public static String getContainerKeyPrefix(long containerID) {
// NOTE: Rocksdb normally needs a fixed length prefix.
- return FixedLengthStringUtils.bytes2String(Longs.toByteArray(containerID));
+ return FixedLengthStringUtils.bytes2String(Longs.toByteArray(containerID))
Review Comment:
Ah, got it. I think if we follow the old way, the separator will be a part of the actual key, for example there is a blockKey, then this separator goes to the head of a blockKey(which is a localID), then things got messed up.
With the new approach, the separator is part of the prefix, even we got 1 byte more to compare, it is fine.
I think the ideal effect in my mind is that, the separator should below to neither the prefix nor the actual key.
Then, even we got the separator changed, we are still likely to parse the keys, for example:
change from '|' -> '#', if we parse the key like:
1. seek with 8bytes prefix
2. skip 1 byte for separator
3. parse the left bytes for blockKey.
Then we could make the separator isolated and independent, of course if you have a separator with different length, then it will not be possible.
So let's just go with this approach and see if we could do better in the future.
--
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
[GitHub] [ozone] ChenSammi commented on pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3420:
URL: https://github.com/apache/ozone/pull/3420#issuecomment-1131019640
cc @guihecheng
--
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
[GitHub] [ozone] guihecheng commented on a diff in pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
guihecheng commented on code in PR #3420:
URL: https://github.com/apache/ozone/pull/3420#discussion_r876566334
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerData.java:
##########
@@ -365,7 +365,7 @@ public String deletingBlockKeyPrefix() {
public KeyPrefixFilter getUnprefixedKeyFilter() {
String schemaPrefix = containerPrefix();
return new KeyPrefixFilter().addFilter(
- schemaPrefix == null ? "#" : schemaPrefix + "#", true);
Review Comment:
Ah, this is actually a fix for the original code(my bad), the `containerPrefix()` never return null.
Nit: If we are checking `schemaPrefix.isEmpty()`, then may be we could just skip the check and do `schemaPrefix + "#"` all over the place?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java:
##########
@@ -137,11 +143,16 @@ public DBColumnFamilyDefinition<String, Long> getMetadataColumnFamily() {
public static String getContainerKeyPrefix(long containerID) {
// NOTE: Rocksdb normally needs a fixed length prefix.
- return FixedLengthStringUtils.bytes2String(Longs.toByteArray(containerID));
+ return FixedLengthStringUtils.bytes2String(Longs.toByteArray(containerID))
Review Comment:
I saw that the original version of this code tries to add this separator to the `KeyValueContainerData.containerPrefix()`, is there a special reason that we moved the separator here?
In my mind, the original version should work, and the low-level rocksdb only have to compare the first 8bytes rather than 9bytes for the current code.
--
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
[GitHub] [ozone] guihecheng commented on a diff in pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
guihecheng commented on code in PR #3420:
URL: https://github.com/apache/ozone/pull/3420#discussion_r877093307
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueBlockIterator.java:
##########
@@ -79,16 +81,30 @@ public class TestKeyValueBlockIterator {
private String datanodeID = UUID.randomUUID().toString();
private String clusterID = UUID.randomUUID().toString();
- public TestKeyValueBlockIterator(ContainerTestVersionInfo versionInfo) {
+ public TestKeyValueBlockIterator(ContainerTestVersionInfo versionInfo,
+ String keySeparator) {
this.layout = versionInfo.getLayout();
this.schemaVersion = versionInfo.getSchemaVersion();
this.conf = new OzoneConfiguration();
ContainerTestVersionInfo.setTestSchemaVersion(schemaVersion, conf);
+ DatanodeConfiguration dc = conf.getObject(DatanodeConfiguration.class);
+ dc.setContainerSchemaV3KeySeparator(keySeparator);
Review Comment:
```suggestion
dc.setContainerSchemaV3KeySeparator(keySeparator);
conf.setFromObject(dc);
```
--
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
[GitHub] [ozone] guihecheng commented on a diff in pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
guihecheng commented on code in PR #3420:
URL: https://github.com/apache/ozone/pull/3420#discussion_r877066607
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueBlockIterator.java:
##########
@@ -79,16 +81,30 @@ public class TestKeyValueBlockIterator {
private String datanodeID = UUID.randomUUID().toString();
private String clusterID = UUID.randomUUID().toString();
- public TestKeyValueBlockIterator(ContainerTestVersionInfo versionInfo) {
+ public TestKeyValueBlockIterator(ContainerTestVersionInfo versionInfo,
+ String keySeparator) {
this.layout = versionInfo.getLayout();
this.schemaVersion = versionInfo.getSchemaVersion();
this.conf = new OzoneConfiguration();
ContainerTestVersionInfo.setTestSchemaVersion(schemaVersion, conf);
+ DatanodeConfiguration dc = conf.getObject(DatanodeConfiguration.class);
+ dc.setContainerSchemaV3KeySeparator(keySeparator);
Review Comment:
Maybe we need to `conf.setFromObject(datanodeConfiguration);` to update the conf 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
[GitHub] [ozone] ChenSammi commented on pull request #3420: HDDS-6747. [Merge rocksdb in datanode] Support configurable separator between container ID and me…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3420:
URL: https://github.com/apache/ozone/pull/3420#issuecomment-1134131428
Thanks @guihecheng .
--
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