-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for Vixie cron local time jump tracking algorithm #7
base: master
Are you sure you want to change the base?
Conversation
The Vixie cron local time jump tracking algorithm changes non-wildcard and wildcard job execution behavior intelligently in the presence of DST transitions and most forward and reverse clock skew occurrences. Implementing this algorithm requires two flags during cron trigger checking, named do_wild and do_non_wild, which filter out certain triggers as needed. The default setting of True for both flags preserves standard behavior.
# matches testex1 when do_wild=True only and not testex2 | ||
tuple2 = (2017, 9, 11, 2, 1) | ||
|
||
self.assertTrue(testex1.check_trigger(tuple1, do_wild=True, do_non_wild=True)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are simple and repetitive enough that they should be parametrized.
test_cases = [
(True, tuple1, True, False),
# ... repeat for each test ...
]
for expected_result, date, wild, non_wild in test_cases:
# ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could do that if it's preferred, it will however cause you to get a stack trace to an arbitrary assertion failure rather than a specific one for a specific failing case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test assertions allow the user to specify custom messages:
>>> unittest.TestCase().assertTrue(False, "my message")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.5/unittest/case.py", line 678, in assertTrue
raise self.failureException(msg)
AssertionError: False is not true : my message
You can add the variables to the message using string formatting. I would be fine with you doing ... % locals()
then you can reference the variables inside the format string like "expected_result = %(expected_result)r, date = %(date)r, ..."
.
@@ -153,13 +153,18 @@ def compute_numtab(self): | |||
elif self.string_tab[4] == "*" and self.string_tab[2] != "*": | |||
self.numerical_tab[4] = set() | |||
|
|||
def check_trigger(self, date_tuple, utc_offset=0): | |||
def check_trigger(self, date_tuple, utc_offset=0, do_wild=True, do_non_wild=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally don't use "do" in my code to describe things. This is a function argument, so it's a given that it probably does something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I named them this, because Vixie cron itself named them this, and I was trying to stay consistent.
# Arriving at this point means the date landed within the constraints | ||
# of all fields; the associated trigger should be fired. | ||
return True | ||
is_wild = self.string_tab[0] == '*' or self.string_tab[1] == '*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I find the change kind of peculiar. Shoving half the logic needed to achieve the skew handling into cronex then expecting the library users to roll their own wrapper feels wrong to me. I think that either cronex should support skewing as a generic feature or it should expose enough state for the caller to decide whether or not to ignore a trigger without changing the existing interface. I need to give this implementation some more thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fair criticism. I was trying to make the most minimal change I could that would solve the use case.
tuple2 = (2017, 9, 11, 2, 1) | ||
|
||
self.assertTrue(testex1.check_trigger(tuple1, do_wild=True, do_non_wild=True)) | ||
self.assertTrue(testex1.check_trigger(tuple1, do_wild=True, do_non_wild=False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a stickler for style. Please stick to the existing convention of wrapping lines after the 79th column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can work on it
I plan to finish the rewrite of Cronex in the next few days, and I'm considering adding general support for clock skews. Would you be willing to show me the code you've written that uses the proposed changes in this pull request so I can see exactly how you handle skewing? |
I'm happy to share how I made it work. Have you got a preferred out-of-band discussion vector of choice? Email? Google Talk? Skype? Facebook Messenger? Etc.? It might be a better route to explain it some more. In general, I did it by implementing a rendition of this very classic algorithm: https://anonscm.debian.org/cgit/pkg-cron/pkg-cron.git/tree/cron.c#n167 I needed the |
Email would be fine with me, but if you would prefer real-time communication, I am reachable over Google Hangouts or we could use IRC. However, I'm going to circle back on this later. I've finished most of the unit tests for the rewritten Cronex, and I think it would be best to hold off on coming up with a design for this feature until after I've pushed a release candidate you can look at; although the interface of the CronExpression is mostly unchanged, the trigger checking logic and the internal representation of expressions has changed dramatically sharing no code with the original version. |
The Vixie cron local time jump tracking algorithm changes non-wildcard and
wildcard job execution behavior intelligently in the presence of DST
transitions and most forward and reverse clock skew occurrences.
Implementing this algorithm requires two flags during cron trigger checking,
named
do_wild
anddo_non_wild
, which filter out certain triggers as needed.The default setting of
True
for both flags preserves standard behavior.