[BEAM-3382] Enforce strictly positive natural numbers for AfterCount timers #4300

mariapython · 2017-12-20T21:38:17Z

No description provided.

mariapython · 2017-12-20T21:38:38Z

aaltay

BEAM-3153 in the title does not seem to be directly related to this change, you can have a different issue for this change.

aaltay · 2017-12-20T22:26:53Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):


Perhaps also have a JIRA issue, so that this can be fixed in other sdks.

aaltay · 2017-12-20T22:26:53Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if count == 0:
+      raise ValueError("count (%s) must be a non-zero natural number." % count)


Make the error statement match the condition. The condition is not checking for natural numbers either. For example 2.3 would not raise this error even though it is not a natural number. You can rephrase it as must be a positive number.

For string formatting of numbers, you can use %d instead of %s.

aaltay · 2017-12-20T22:26:53Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if count == 0:


This is checking for non-zero numbers instead of the positive number. You can change this to count > 0 to check for positive numbers.

mariapython

Thanks, Ahmet. PTAL.

aaltay wrote:
BEAM-3153 in the title does not seem to be directly related to this change, you can have a different issue for this change.

Done.

mariapython · 2017-12-21T08:16:35Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if count == 0:


aaltay wrote:
This is checking for non-zero numbers instead of the positive number. You can change this to count > 0 to check for positive numbers.

Done.

mariapython · 2017-12-21T08:16:35Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if count == 0:
+      raise ValueError("count (%s) must be a non-zero natural number." % count)


aaltay wrote:

Make the error statement match the condition. The condition is not checking for natural numbers either. For example 2.3 would not raise this error even though it is not a natural number. You can rephrase it as must be a positive number.

For string formatting of numbers, you can use %d instead of %s.

The reason why I didn't check for non-int was that the proto enforced integers, so 2.3 wouldn't be accepted. But the message was not clear, and as you say, it is cleaner to match the error statement, so I modified it per your suggestion.

mariapython · 2017-12-21T08:16:35Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):


aaltay wrote:
Perhaps also have a JIRA issue, so that this can be fixed in other sdks.

I am not sure if it affects other SDKs, but I have now filed BEAM-3382 for sdk-py-core

aaltay · 2017-12-21T18:07:46Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if not isinstance(count, int) or count < 1:


isinstance(count, int) is over limiting, For example both isinstance(3l, int) and isinstance(3.0, int) would result in False. Even though there is no reason to accept them as valid arguments. I do not not think we need to type check things. Checking for the count < 1 should be enough here.

mariapython

Thanks, PTAL.

mariapython · 2017-12-21T22:20:35Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if not isinstance(count, int) or count < 1:


aaltay wrote:
isinstance(count, int) is over limiting, For example both isinstance(3l, int) and isinstance(3.0, int) would result in False. Even though there is no reason to accept them as valid arguments. I do not not think we need to type check things. Checking for the count < 1 should be enough here.

I am not sure if you mean count = "thirty one" or count = "three followed by letter L"
The first is rightfully True and the second is rightfully False, so I don't see the problem with that statement.
count < 1 is not restrictive enough, a user could enter 3.4 and they would only get an obscure message thrown by the protobuf. With the checking above, they get a clear reason of their error.

I meant a long value as in type(3L) = <type 'long'>. I do not see any reason for not letting users to pass long values here.

mariapython

Thanks, PTAL.

mariapython · 2017-12-22T09:13:19Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if not isinstance(count, int) or count < 1:


aaltay wrote:
I meant a long value as in type(3L) = <type 'long'>. I do not see any reason for not letting users to pass long values here.

It makes sense. I have just incorporated the right change so that long types are allowed while enforcing non-fractional numbers.

mariapython

Thanks, PTAL.

mariapython · 2017-12-22T09:14:28Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if not isinstance(count, int) or count < 1:


aaltay wrote:
I meant a long value as in type(3L) = <type 'long'>. I do not see any reason for not letting users to pass long values here.

It makes sense. I have just incorporated the right change so that long types are allowed while enforcing non-fractional numbers.

aaltay

mariapython wrote:
Thanks, PTAL.

Done.

aaltay · 2017-12-22T18:48:09Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if not isinstance(count, int) or count < 1:


mariapython wrote:
It makes sense. I have just incorporated the right change so that long types are allowed while enforcing non-fractional numbers.

This will still fail for float types like 3.0 but I think that is fine, we do not need to make more changes here.

aaltay · 2017-12-22T18:48:09Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if count == 0:
+      raise ValueError("count (%s) must be a non-zero natural number." % count)


mariapython wrote:
The reason why I didn't check for non-int was that the proto enforced integers, so 2.3 wouldn't be accepted. But the message was not clear, and as you say, it is cleaner to match the error statement, so I modified it per your suggestion.

Could you change there error string to count (%d) must be a positive integer. ?

aaltay · 2017-12-22T18:48:09Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):


mariapython wrote:
I am not sure if it affects other SDKs, but I have now filed BEAM-3382 for sdk-py-core

Done.

mariapython · 2017-12-23T00:26:17Z

sdks/python/apache_beam/transforms/trigger.py

@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
  COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

  def __init__(self, count):
+    if count == 0:
+      raise ValueError("count (%s) must be a non-zero natural number." % count)


aaltay wrote:
Could you change there error string to count (%d) must be a positive integer. ?

Done.

aaltay · 2017-12-23T01:08:16Z

LGTM, I can merge after tests pass.

aaltay · 2017-12-23T18:26:13Z

retest this please

aaltay · 2017-12-23T18:27:36Z

Tests seemingly failed for unrelated timeouts. I want to rerun tests again. Asked Jenkins to re-trigger the tests.

aaltay reviewed Dec 20, 2017

View reviewed changes

mariapython changed the title ~~[BEAM-3153] Enforce strictly positive natural numbers for AfterCount timers~~ [BEAM-3382] Enforce strictly positive natural numbers for AfterCount timers Dec 21, 2017

mariapython force-pushed the direct-timers-test branch from 17604fb to b7de0ec Compare December 21, 2017 07:56

mariapython commented Dec 21, 2017

View reviewed changes

aaltay reviewed Dec 21, 2017

View reviewed changes

mariapython commented Dec 21, 2017

View reviewed changes

mariapython force-pushed the direct-timers-test branch from 2df1e91 to 114fddf Compare December 22, 2017 09:11

mariapython commented Dec 22, 2017

View reviewed changes

aaltay reviewed Dec 22, 2017

View reviewed changes

Address review comment

18c4162

mariapython commented Dec 23, 2017

View reviewed changes

mariapython force-pushed the direct-timers-test branch from 114fddf to 18c4162 Compare December 23, 2017 00:37

aaltay merged commit 312c4ab into apache:master Dec 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-3382] Enforce strictly positive natural numbers for AfterCount timers #4300

[BEAM-3382] Enforce strictly positive natural numbers for AfterCount timers #4300

mariapython commented Dec 20, 2017

mariapython commented Dec 20, 2017

aaltay left a comment

aaltay Dec 20, 2017

aaltay Dec 20, 2017

aaltay Dec 20, 2017

mariapython left a comment

mariapython Dec 21, 2017

mariapython Dec 21, 2017

mariapython Dec 21, 2017

aaltay Dec 21, 2017

mariapython left a comment

mariapython Dec 21, 2017

aaltay Dec 21, 2017

mariapython left a comment

mariapython Dec 22, 2017

mariapython left a comment

mariapython Dec 22, 2017

aaltay left a comment

aaltay Dec 22, 2017

aaltay Dec 22, 2017

aaltay Dec 22, 2017

mariapython Dec 23, 2017

aaltay commented Dec 23, 2017

aaltay commented Dec 23, 2017

aaltay commented Dec 23, 2017

		@@ -378,6 +378,8 @@ class AfterCount(TriggerFn):
		COUNT_TAG = _CombiningValueStateTag('count', combiners.CountCombineFn())

		def __init__(self, count):

[BEAM-3382] Enforce strictly positive natural numbers for AfterCount timers #4300

[BEAM-3382] Enforce strictly positive natural numbers for AfterCount timers #4300

Conversation

mariapython commented Dec 20, 2017

mariapython commented Dec 20, 2017

aaltay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariapython left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariapython left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariapython left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariapython left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaltay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaltay commented Dec 23, 2017

aaltay commented Dec 23, 2017

aaltay commented Dec 23, 2017