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/06/09 23:29:22 UTC
[GitHub] [ozone] aswinshakil opened a new pull request, #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
aswinshakil opened a new pull request, #3498:
URL: https://github.com/apache/ozone/pull/3498
## What changes were proposed in this pull request?
To validate server-side replication configuration `ozone.server.default.replication` and `ozone.server.default.replication.type` during Ozone Manager startup.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6841
## How was this patch tested?
This patch was tested manually and using Unit tests.
--
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] umamaheswararao commented on a diff in pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3498:
URL: https://github.com/apache/ozone/pull/3498#discussion_r894756542
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfigValidator.java:
##########
@@ -50,10 +50,11 @@ public void init() {
public ReplicationConfig validate(ReplicationConfig replicationConfig) {
if (validationRegexp != null) {
- if (!validationRegexp.matcher(replicationConfig.toString()).matches()) {
+ if (!validationRegexp.matcher(
+ replicationConfig.configFormat()).matches()) {
throw new IllegalArgumentException("Invalid replication config " +
- replicationConfig + ". Replication config should match the "
- + validationPattern + " pattern.");
+ replicationConfig.configFormat() + ". Replication config " +
+ "should match the " + validationPattern + " pattern.");
Review Comment:
I am worried on displaying this validationPatten in log. Are we allowing just EC-3-2 ? without chunk size in string? ( If not, we will get another bug about this log saying " this log is confusing" :-) )
Probably we can allow in ECReplicationConfig and just take 1024K as default for chunkSize?
--
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] umamaheswararao commented on a diff in pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3498:
URL: https://github.com/apache/ozone/pull/3498#discussion_r897067629
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -516,6 +517,8 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption)
BucketLayout.OBJECT_STORE + ", " + BucketLayout.LEGACY + ".");
}
+ // Validates the default server-side replication configs.
+ this.defaultReplicationConfig = getDefaultReplicationConfig();
Review Comment:
One more small change I should have noted to you here. When you use getDefaultReplicationConfig, it can return this,defaultReplicationConfig if it's not null right? So, every time we don't need to do the validation when getDefaultReplicationConfig called.
--
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] aswinshakil commented on a diff in pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3498:
URL: https://github.com/apache/ozone/pull/3498#discussion_r894764892
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -608,6 +612,43 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption)
}
}
Review Comment:
We can do this. But `getDefaultReplicationConfig()` already does a validate on `validator.validate(defaultRepConfig)`. Also it does a substition with client side replication config if the server-side field are `null`(Which might not happen as we have server-side defaults).
--
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] umamaheswararao commented on a diff in pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3498:
URL: https://github.com/apache/ozone/pull/3498#discussion_r894753237
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -608,6 +612,43 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption)
}
}
Review Comment:
There is a method getDefaultReplicationConfig, which is already parsing and getting the repConfig.
Probably we can just use that method and then validate that?
Since they are fixed once loaded, why don't we cave them in OzoneManager?
Let's say in OzoneManager:
ReplicationConfig defaultRepConfig = null;
on startUp, where bucketLayouts validated,
defaultRepConfig = getDefaultReplicationConfig();
ReplicationConfigValidator validator =
conf.getObject(ReplicationConfigValidator.class);
validator.validate(defaultRepConfig)
Can we do this?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfigValidator.java:
##########
@@ -50,10 +50,11 @@ public void init() {
public ReplicationConfig validate(ReplicationConfig replicationConfig) {
if (validationRegexp != null) {
- if (!validationRegexp.matcher(replicationConfig.toString()).matches()) {
+ if (!validationRegexp.matcher(
Review Comment:
@kerneltime I think the idea here is to restrict the non-tested combination numbers. So, the list is becoming like a fixed string. However that pattern is dev configurable.
@Config(key = "allowed-configs",
defaultValue = "^((STANDALONE|RATIS)/(ONE|THREE))|(EC/(3-2|6-3|10-4))$",
If we have confidence with other numbers let's say 12-4, then we can simply update above allowed-configs pattern. IIRC, this was the idea.
--
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] umamaheswararao commented on pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3498:
URL: https://github.com/apache/ozone/pull/3498#issuecomment-1154687089
Looks like this patch has conflicts. You may want to rebase on latest code and resolve conflicts? 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] umamaheswararao merged pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
umamaheswararao merged PR #3498:
URL: https://github.com/apache/ozone/pull/3498
--
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] kerneltime commented on a diff in pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3498:
URL: https://github.com/apache/ozone/pull/3498#discussion_r894136806
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfigValidator.java:
##########
@@ -50,10 +50,11 @@ public void init() {
public ReplicationConfig validate(ReplicationConfig replicationConfig) {
if (validationRegexp != null) {
- if (!validationRegexp.matcher(replicationConfig.toString()).matches()) {
+ if (!validationRegexp.matcher(
Review Comment:
The change looks good. A minor observation: We accept a regular expression as a configuration to limit the replication configurations, it would be useful to validate the regular expression? If we only support a subset of replication configurations would it be better to not validate via string comparison as we already have `ReplicationConfiguration` which has the full fidelity of the configuration already parsed into its constituents, it would be better to validate semantically vs via regular expressions.
--
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] aswinshakil commented on a diff in pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3498:
URL: https://github.com/apache/ozone/pull/3498#discussion_r894766256
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfigValidator.java:
##########
@@ -50,10 +50,11 @@ public void init() {
public ReplicationConfig validate(ReplicationConfig replicationConfig) {
if (validationRegexp != null) {
- if (!validationRegexp.matcher(replicationConfig.toString()).matches()) {
+ if (!validationRegexp.matcher(
+ replicationConfig.configFormat()).matches()) {
throw new IllegalArgumentException("Invalid replication config " +
- replicationConfig + ". Replication config should match the "
- + validationPattern + " pattern.");
+ replicationConfig.configFormat() + ". Replication config " +
+ "should match the " + validationPattern + " pattern.");
Review Comment:
We can allow 1024K but it would heavily restrict the chunk size to only a few values. Is that what we want?
--
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] umamaheswararao commented on a diff in pull request #3498: HDDS-6841. EC: Validate the server default configuration on Ozone manager startup
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3498:
URL: https://github.com/apache/ozone/pull/3498#discussion_r895986232
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -608,6 +614,43 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption)
}
}
+ private void validateReplicationConfigs(OzoneConfiguration conf) {
Review Comment:
is this method still needed?
--
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