ibzib commented on a change in pull request #14938:
URL: https://github.com/apache/beam/pull/14938#discussion_r646920637
##########
File path: sdks/python/apache_beam/options/pipeline_options.py
##########
@@ -934,8 +934,7 @@ def validate(self, validator):
errors.extend(validator.validate_sdk_container_image_options(self))
if validator.is_service_runner():
- errors.extend(
- validator.validate_optional_argument_positive(self, 'num_workers'))
+ errors.extend(validator.validate_worker_num(self))
Review comment:
Nit: function name should be consistent with option names where
possible. Helps with readability as well as grep.
```suggestion
errors.extend(validator.validate_num_workers(self))
```
##########
File path: sdks/python/apache_beam/options/pipeline_options_validator.py
##########
@@ -250,6 +252,20 @@ def validate_sdk_container_image_options(self, view):
return errors
+ def validate_worker_num(self, view):
+ """Validates that Dataflow worker number is valid."""
+ errors = self.validate_optional_argument_positive(view, 'num_workers')
Review comment:
Should we validate that max_num_workers is positive (if set) as well?
##########
File path: sdks/python/apache_beam/options/pipeline_options_validator.py
##########
@@ -110,6 +110,8 @@ class PipelineOptionsValidator(object):
'Option environment_config is incompatible with option(s) %s.')
ERR_MISSING_REQUIRED_ENVIRONMENT_OPTION = (
'Option %s is required for environment type %s.')
+ ERR_WORKER_NUM_TOO_HIGH = (
Review comment:
Same here.
```suggestion
ERR_NUM_WORKERS_TOO_HIGH = (
```
##########
File path: sdks/python/apache_beam/options/pipeline_options_validator_test.py
##########
@@ -370,6 +372,50 @@ def test_validate_dataflow_job_file(self):
errors = validator.validate()
self.assertFalse(errors)
+ def test_worker_num_is_positive(self):
Review comment:
Same here (and for other test functions).
```suggestion
def test_num_workers_is_positive(self):
```
--
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:
[email protected]
