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/09 03:05:17 UTC
[GitHub] [ozone] kaijchen opened a new pull request, #3391: HDDS-6710. Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
kaijchen opened a new pull request, #3391:
URL: https://github.com/apache/ozone/pull/3391
## What changes were proposed in this pull request?
Add `OZONE_SCM_EC_PIPELINE_LIMIT` in SCMConfigKeys to support setting EC pipeline limit in MiniOzoneCluster.
The config key `ozone.scm.ec.pipeline.minimum` was changed to `ozone.scm.ec.pipeline.limit`.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6710
## How was this patch tested?
Integration test in other (developing) branch has verified this config is effective.
(By creating a MiniOzoneCluster and write some EC keys to it, then check the number of containers created.)
--
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] kaijchen commented on pull request #3391: HDDS-6710. Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3391:
URL: https://github.com/apache/ozone/pull/3391#issuecomment-1120588395
@sodonnel PTAL, 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.
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] adoroszlai commented on pull request #3391: HDDS-6710. EC: Add EC pipeline minimum to MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3391:
URL: https://github.com/apache/ozone/pull/3391#issuecomment-1124837753
Thanks @kaijchen. Also, I know that there are existing config setting methods in the builder that IMO shouldn't exist. I think it should be cleaned up a bit (either in HDDS-6713 or a pre-factoring task).
--
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] sodonnel commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870300471
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -864,6 +864,14 @@
of max amount of pipelines which are OPEN.
</description>
</property>
+ <property>
Review Comment:
I believe you can do something like this:
```
WritableECContainerProvider.WritableECContainerProviderConfig ecConf
= conf.getObject(WritableECContainerProvider.WritableECContainerProviderConfig.class);
ecConf.setMinimumPipelines(10);
conf.setFromObject(ecConf);
```
As I said earlier, must more complex than just a simple constant and default. I have seen new code added using the old way too. I feel the project is in a bad place with having two different ways to set the config, but its a big effort to move everything to the new way, and I am not sure everyone agrees the new way is better.
--
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] adoroszlai commented on pull request #3391: HDDS-6710. EC: Add EC pipeline minimum to MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3391:
URL: https://github.com/apache/ozone/pull/3391#issuecomment-1124725624
Thanks @kaijchen for working on this.
I'm not sure we should add new methods to `MiniOzoneCluster.Builder` to set config properties, unless it is a widely used setting with different values in various tests.
When building a `MiniOzoneCluster`, tests provide an `OzoneConfiguration`, which they can already customize.
If the production default value is not suitable for test environment, we can override it for all integration tests in `hadoop-ozone/integration-test/src/test/resources/ozone-site.xml`.
Can you please share more details on the expected usage of this property?
--
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] sodonnel commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870145153
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -367,6 +367,10 @@ public final class ScmConfigKeys {
// Setting to zero by default means this limit doesn't take effect.
public static final int OZONE_SCM_RATIS_PIPELINE_LIMIT_DEFAULT = 0;
+ public static final String OZONE_SCM_EC_PIPELINE_LIMIT =
+ "ozone.scm.ec.pipeline.limit";
Review Comment:
Same comment as below - keep the name as OZONE_SCM_EC_PIPELINE_MINIMUM.
--
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] kaijchen commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870166341
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -864,6 +864,14 @@
of max amount of pipelines which are OPEN.
</description>
</property>
+ <property>
Review Comment:
Thanks for explaining. How do we change the config in the "new way"?
More specifically, how to set this value in MiniOzoneCluster?
--
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] kaijchen commented on pull request #3391: HDDS-6710. EC: Add EC pipeline minimum to MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3391:
URL: https://github.com/apache/ozone/pull/3391#issuecomment-1124731223
> Thanks @kaijchen for working on this.
>
> I'm not sure we should add new methods to `MiniOzoneCluster.Builder` to set config properties, unless it is a widely used setting with different values in various tests.
>
> When building a `MiniOzoneCluster`, tests provide an `OzoneConfiguration`, which they can already customize.
>
> If the production default value is not suitable for test environment, we can override it for all integration tests in `hadoop-ozone/integration-test/src/test/resources/ozone-site.xml`.
>
> Can you please share more details on the expected usage of this property?
I was working on EC offline recovery in my local branch, and I need to verify interfaces like `ListBlock` and `ReadChunk`.
I limit EC pipeline number to 1 in tests to ensure all the data is written into the same container group.
--
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] kaijchen commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870166341
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -864,6 +864,14 @@
of max amount of pipelines which are OPEN.
</description>
</property>
+ <property>
Review Comment:
Thanks for explaining. How do we change the config in the "new way"?
More specifically, how to set this value in `MiniOzoneClusterImpl`?
--
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] adoroszlai commented on pull request #3391: HDDS-6710. EC: Add EC pipeline minimum to MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3391:
URL: https://github.com/apache/ozone/pull/3391#issuecomment-1124830418
Thanks @kaijchen for the details. I think it's better to set custom `ozone.scm.ec.pipeline.minimum` in the test setup then, similar to this:
https://github.com/apache/ozone/blob/5459b49538f16256a4e0f7d3c7f2981d0d7a9fae/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestBlockOutputStream.java#L84-L102
Part of the reason I would like to avoid cluttering the builder with config settings is HDDS-6713, since all such methods need to be overridden in subclass builder.
But even if we had a single builder class, I don't think each config setting needs its own setter method.
--
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] sodonnel commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870163396
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -864,6 +864,14 @@
of max amount of pipelines which are OPEN.
</description>
</property>
+ <property>
Review Comment:
We the config is defined as it is in ECWriteableContainerProvider, using the "Type Safe Config" definitions, at compile time, any config with those annotations are added into a ozone-default-generated.xml file, so we don't need to update the ozone-default.xml with this value.
The problem we have, is that half the project uses the old way (SCMConfigKeys and ozone-default.xml) and half uses the new way. The project direction is to adopt the new way, although I personally prefer the old way, as it is simpler and easier to understand :-)
As the config is defined in the "new way" here, I think we should remove the additions to ozone-default.xml and ScmConfigKeys.
--
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] sodonnel commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870144358
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -218,13 +218,14 @@ private long getConfiguredContainerSize() {
@ConfigGroup(prefix = "ozone.scm.ec")
public static class WritableECContainerProviderConfig {
- @Config(key = "pipeline.minimum",
- defaultValue = "5",
+ @Config(key = "pipeline.limit",
Review Comment:
This parameter was not intended to be the limit, but the minimum number of pipelines we should have so we don't only have one pipeline and always allocate blocks to the same one.
We need to finish off HDDS-5327 and decide on a strategy based on cluster size and load to determine the pipeline limit.
As the code stands at the moment, this parameter indicates the number of pipelines that will end up open at any given time. It is like a minimum and a limit. Once we figure out how to handle the limit, then this parameter will be the minimum number of pipelines. Therefore it probably makes sense to keep the parameter named pipeline.minimum rathern than limit.
--
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] kaijchen commented on pull request #3391: HDDS-6710. EC: Add EC pipeline minimum to MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3391:
URL: https://github.com/apache/ozone/pull/3391#issuecomment-1124832078
Thanks for the info @adoroszlai, I will close this.
--
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] kaijchen commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870147078
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -218,13 +218,14 @@ private long getConfiguredContainerSize() {
@ConfigGroup(prefix = "ozone.scm.ec")
public static class WritableECContainerProviderConfig {
- @Config(key = "pipeline.minimum",
- defaultValue = "5",
+ @Config(key = "pipeline.limit",
Review Comment:
Thanks for the explanation. I'll change it back.
--
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] adoroszlai commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870332087
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -864,6 +864,14 @@
of max amount of pipelines which are OPEN.
</description>
</property>
+ <property>
Review Comment:
Example with `SCMClientConfig` can be found just a few lines below the current change in `MiniOzoneClusterImpl`.
https://github.com/apache/ozone/blob/88b5e854be89be7ef3944527be4b87d456ee297b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java#L692-L695
--
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] kaijchen closed pull request #3391: HDDS-6710. EC: Add EC pipeline minimum to MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
kaijchen closed pull request #3391: HDDS-6710. EC: Add EC pipeline minimum to MiniOzoneCluster
URL: https://github.com/apache/ozone/pull/3391
--
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] kaijchen commented on a diff in pull request #3391: HDDS-6710. EC: Add EC pipeline limit to SCMConfigKeys and MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3391:
URL: https://github.com/apache/ozone/pull/3391#discussion_r870339176
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -864,6 +864,14 @@
of max amount of pipelines which are OPEN.
</description>
</property>
+ <property>
Review Comment:
Thanks @sodonnel and @adoroszlai for the info.
--
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