You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/03/10 22:42:19 UTC
[GitHub] [helix] narendly opened a new pull request #883: Support
enableCompression in workflow and job configs
narendly opened a new pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883
Previously, there was no way for users to set enableCompression in workflows and jobs configs. This allows them to set that field in the Builder.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #883: Support
enableCompression in workflow and job configs
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#discussion_r391763665
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobConfig.java
##########
@@ -553,8 +556,11 @@ public static Builder fromMap(Map<String, String> cfg) {
}
if (cfg.containsKey(JobConfigProperty.RebalanceRunningTask.name())) {
b.setRebalanceRunningTask(
- Boolean.valueOf(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
+ Boolean.parseBoolean(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
}
+ // If not set, parseBoolean returns false for null
+ b.setEnableCompression(
Review comment:
Then we should do a check here, right? If get null, we will get exception for parse boolean.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #883: Support
enableCompression in workflow and job configs
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#discussion_r391247879
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowConfig.java
##########
@@ -552,6 +559,11 @@ public Builder setConfigMap(Map<String, String> cfg) {
}
}
+ if (cfg.containsKey(ZNRecord.ENABLE_COMPRESSION_BOOLEAN_FIELD)) {
Review comment:
No need to check containsKey, unless the default value enableCompression is true.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on issue #883: Support enableCompression
in workflow and job configs
Posted by GitBox <gi...@apache.org>.
narendly commented on issue #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#issuecomment-597372487
> Do we want to just add this field or support a builder can carry over all the fields from JobConfig?
Just this field only because we want to curb the way user creates workflows and jobs now. Users should ideally use PropertyStore to store user-generated custom fields.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #883: Support
enableCompression in workflow and job configs
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#discussion_r391247480
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobConfig.java
##########
@@ -553,7 +556,11 @@ public static Builder fromMap(Map<String, String> cfg) {
}
if (cfg.containsKey(JobConfigProperty.RebalanceRunningTask.name())) {
b.setRebalanceRunningTask(
- Boolean.valueOf(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
+ Boolean.parseBoolean(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
+ }
+ if (cfg.containsKey(ZNRecord.ENABLE_COMPRESSION_BOOLEAN_FIELD)) {
Review comment:
It is redundant to check `containsKey`: if the value is not set, parseBoolean still returns false.
Unless the default value enableCompression is true, but I don't think this should be the case.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly merged pull request #883: Support
enableCompression in workflow and job configs
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #883:
Support enableCompression in workflow and job configs
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#discussion_r391799625
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobConfig.java
##########
@@ -553,8 +556,11 @@ public static Builder fromMap(Map<String, String> cfg) {
}
if (cfg.containsKey(JobConfigProperty.RebalanceRunningTask.name())) {
b.setRebalanceRunningTask(
- Boolean.valueOf(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
+ Boolean.parseBoolean(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
}
+ // If not set, parseBoolean returns false for null
+ b.setEnableCompression(
Review comment:
@dasahcc
This is the implementation for parseBoolean() if the comment wasn't clear:
```
public static boolean parseBoolean(String s) {
return ((s != null) && s.equalsIgnoreCase("true"));
}
```
Do you see why the null check is not necessary?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on issue #883: Support enableCompression
in workflow and job configs
Posted by GitBox <gi...@apache.org>.
narendly commented on issue #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#issuecomment-598015461
> > > Do we want to just add this field or support a builder can carry over all the fields from JobConfig?
> >
> >
> > Just this field only because we want to curb the way user creates workflows and jobs now. Users should ideally use PropertyStore to store user-generated custom fields.
>
> Can we add a log to tell users their properties are ignored if any? I think that would be helpful for the end users.
I don't think adding logs is necessary. Logs are helpful for leaving historical information that can't be found anywhere else. In this case, however, users can easily check whether their fields ever made it to the znode by inspecting the znode.
Also, we want to limit the kind of fields we allow users to include in workflow and job config znodes by having them use Builders.
The biggest worry of all is again for heavy users of Task Framework, this might pollute the log.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #883: Support
enableCompression in workflow and job configs
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#discussion_r392419672
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobConfig.java
##########
@@ -553,8 +556,11 @@ public static Builder fromMap(Map<String, String> cfg) {
}
if (cfg.containsKey(JobConfigProperty.RebalanceRunningTask.name())) {
b.setRebalanceRunningTask(
- Boolean.valueOf(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
+ Boolean.parseBoolean(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
}
+ // If not set, parseBoolean returns false for null
+ b.setEnableCompression(
Review comment:
OK. The null should be final. But it make this entry explicitly in ZNRecord, right? Usually, our check is if the entry is not in ZNRecord, we use default value. But we dont carry the default value and explicitly set in ZNRecord to reduce the size of data set.
In this case, it is better we follow our convention: do the check when we carry it over.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #883:
Support enableCompression in workflow and job configs
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#discussion_r392557853
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobConfig.java
##########
@@ -553,8 +556,11 @@ public static Builder fromMap(Map<String, String> cfg) {
}
if (cfg.containsKey(JobConfigProperty.RebalanceRunningTask.name())) {
b.setRebalanceRunningTask(
- Boolean.valueOf(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
+ Boolean.parseBoolean(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
}
+ // If not set, parseBoolean returns false for null
+ b.setEnableCompression(
Review comment:
Okay I see what you mean. I will revert to the original pattern.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on issue #883: Support enableCompression in
workflow and job configs
Posted by GitBox <gi...@apache.org>.
pkuwm commented on issue #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#issuecomment-597845883
> > Do we want to just add this field or support a builder can carry over all the fields from JobConfig?
>
> Just this field only because we want to curb the way user creates workflows and jobs now. Users should ideally use PropertyStore to store user-generated custom fields.
Can we add a log to tell users their properties are ignored if any? I think that would be helpful for the end users.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #883:
Support enableCompression in workflow and job configs
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #883: Support enableCompression in workflow and job configs
URL: https://github.com/apache/helix/pull/883#discussion_r391799924
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobConfig.java
##########
@@ -553,8 +556,11 @@ public static Builder fromMap(Map<String, String> cfg) {
}
if (cfg.containsKey(JobConfigProperty.RebalanceRunningTask.name())) {
b.setRebalanceRunningTask(
- Boolean.valueOf(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
+ Boolean.parseBoolean(cfg.get(JobConfigProperty.RebalanceRunningTask.name())));
}
+ // If not set, parseBoolean returns false for null
+ b.setEnableCompression(
Review comment:
```
public static boolean parseBoolean(String s) {
return ((s != null) && s.equalsIgnoreCase("true"));
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org