You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "jojochuang (via GitHub)" <gi...@apache.org> on 2024/07/31 22:14:10 UTC
[PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
jojochuang opened a new pull request, #7012:
URL: https://github.com/apache/ozone/pull/7012
## What changes were proposed in this pull request?
HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock.
The layout version check for incremental chunk list support is performed at DataNode startup only, which means that DataNode would reject any such clients until the next restart after finalization.
The check should be moved later inside PutBlocks handling.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11259
## How was this patch 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.
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #7012:
URL: https://github.com/apache/ozone/pull/7012#discussion_r1702072737
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -162,6 +150,14 @@ public long persistPutBlock(KeyValueContainer container,
}
}
+ boolean incrementalEnabled = true;
+ if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.HBASE_SUPPORT)) {
+ if (isPartialChunkList(data)) {
+ throw new UnsupportedOperationException("DataNode has not finalized " +
Review Comment:
yeah agreed. I think there are DataNode layout upgrade test code somewhere. Let me find out.
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #7012:
URL: https://github.com/apache/ozone/pull/7012#discussion_r1699762060
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -162,7 +157,13 @@ public long persistPutBlock(KeyValueContainer container,
}
}
- db.getStore().putBlockByID(batch, incrementalEnabled, localID, data,
+ if (incrementalEnabled.get() && !VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.HBASE_SUPPORT)) {
Review Comment:
But since OM also checks upgrade finalize before and call will fail in OM itself, So in this case client will not call DN and I think this check is not required in DN in case of `finalizeBlock.`
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #7012:
URL: https://github.com/apache/ozone/pull/7012#discussion_r1699905016
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -162,7 +157,13 @@ public long persistPutBlock(KeyValueContainer container,
}
}
- db.getStore().putBlockByID(batch, incrementalEnabled, localID, data,
+ if (incrementalEnabled.get() && !VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.HBASE_SUPPORT)) {
Review Comment:
Agree. We should add the HDDSLayoutFeature.HBASE_SUPPORT check in finalizeBlock too. Although there is check in OM, the check in DN is also preferred. It's like the double insurance.
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #7012:
URL: https://github.com/apache/ozone/pull/7012#discussion_r1699870501
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -162,7 +157,13 @@ public long persistPutBlock(KeyValueContainer container,
}
}
- db.getStore().putBlockByID(batch, incrementalEnabled, localID, data,
+ if (incrementalEnabled.get() && !VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.HBASE_SUPPORT)) {
+ LOG.warn("DataNode has not finalized upgrading to a version that " +
+ "supports incremental chunk list. Fallback to full chunk list");
+ incrementalEnabled.set(false);
Review Comment:
There is chance that if pubBlock is first called before HDDSLayoutFeature.HBASE_SUPPORT is finalized, then incrementalEnabled will be set as false. In this case, incrementalEnabled will not come back as "true" without the DN restart.
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #7012:
URL: https://github.com/apache/ozone/pull/7012#discussion_r1702016689
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -162,6 +150,14 @@ public long persistPutBlock(KeyValueContainer container,
}
}
+ boolean incrementalEnabled = true;
+ if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.HBASE_SUPPORT)) {
+ if (isPartialChunkList(data)) {
+ throw new UnsupportedOperationException("DataNode has not finalized " +
Review Comment:
I'm not sure the exception is correct here. On the client side we use the unchecked `UnsupportedOperationException` because it OK to crash the client at that point. On the server we want to throw an exception type and result code that will be translated back to the client. I think this would be `StorageContainerException` with a new result code similar to `NOT_SUPPORTED_OPERATION_PRIOR_FINALIZATION` which exists on the OM.
Note that there's no tests in the PR. Per the mailing thread it looks like we are going to do those after the merge which is ok, but we should try to make sure this is working correctly here because I think this is the first time we have rejected a request based on a layout version on 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.
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #7012:
URL: https://github.com/apache/ozone/pull/7012#discussion_r1702133122
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -162,6 +150,14 @@ public long persistPutBlock(KeyValueContainer container,
}
}
+ boolean incrementalEnabled = true;
+ if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.HBASE_SUPPORT)) {
+ if (isPartialChunkList(data)) {
+ throw new UnsupportedOperationException("DataNode has not finalized " +
Review Comment:
Opening a new jira to track adding the test code to validate layout version upgrade behavior. https://issues.apache.org/jira/browse/HDDS-11270
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #7012:
URL: https://github.com/apache/ozone/pull/7012
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #7012:
URL: https://github.com/apache/ozone/pull/7012#discussion_r1699717288
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -162,7 +157,13 @@ public long persistPutBlock(KeyValueContainer container,
}
}
- db.getStore().putBlockByID(batch, incrementalEnabled, localID, data,
+ if (incrementalEnabled.get() && !VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.HBASE_SUPPORT)) {
Review Comment:
`finalizeBlock(..)` in `BlockManagerImpl` can also have this check and return error before finalize.
```
if (!VersionedDatanodeFeatures.isFinalized(
HDDSLayoutFeature.HBASE_SUPPORT)) { }
```
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #7012:
URL: https://github.com/apache/ozone/pull/7012#issuecomment-2264187525
Updated the PR. Address review comments.
1. reject finalizeBlock if upgrade is not finalized.
2. Remove DataNode-side ozone.incremental.chunk.list configuration.
3. Reject client if it attempts to send chunks in incremental chunk list format before upgrade finalizes. Previously the DataNode would fallback and treat such request as full chunks, which is wrong.
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #7012:
URL: https://github.com/apache/ozone/pull/7012#issuecomment-2262690718
BTW, before the "HDDSLayoutFeature.HBASE_SUPPORT" is finalized, the FINALIZE_BLOCKS and LAST_CHUNK_INFO column family will be created by DN due to the RocksDB option setCreateMissingColumnFamilies(true), which I think is not perfect, but is fine.
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #7012:
URL: https://github.com/apache/ozone/pull/7012#issuecomment-2263413941
> BTW, before the "HDDSLayoutFeature.HBASE_SUPPORT" is finalized, the FINALIZE_BLOCKS and LAST_CHUNK_INFO column family will be created by DN due to the RocksDB option setCreateMissingColumnFamilies(true), which I think is not perfect, but is fine.
Yes IIRC this is true with all layout features that add new column families. The older versions just ignore them.
--
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #7012:
URL: https://github.com/apache/ozone/pull/7012#discussion_r1699762060
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -162,7 +157,13 @@ public long persistPutBlock(KeyValueContainer container,
}
}
- db.getStore().putBlockByID(batch, incrementalEnabled, localID, data,
+ if (incrementalEnabled.get() && !VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.HBASE_SUPPORT)) {
Review Comment:
But since OM also checks upgrade finalize before and call will fail in OM itself, So in this case client will not call DN and I think this check is not required in 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.
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
Re: [PR] HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #7012:
URL: https://github.com/apache/ozone/pull/7012#issuecomment-2266164454
Merged. Thanks @errose28 and @ashishkumar50.
--
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