Timestamp generator#681
Conversation
|
LGTM Jim. Can you give a try to our benchmark scripts to see how the lock in that generator affects the performance? |
|
|
||
| def _maybe_warn(self, now): | ||
| # should be called from inside the self.lock. | ||
| diff = now - self._last_warn |
There was a problem hiding this comment.
I think this line should be:
diff = self.last - now
as this value may be logged below
| since_last_warn = now - self._last_warn | ||
|
|
||
| warn = (self.warn_on_drift and | ||
| (diff >= self.warning_threshold) and |
There was a problem hiding this comment.
This comparison would be:
self.last + 1 - now >= self.warning_threshold
as the comparison should be done with the value that is going to be returned right?
There was a problem hiding this comment.
I think it should be since_last_warn >= self.warning_threshold -- we're trying to avoid flooding logs, so we care about how long it's been since the last warning was issued. Sound right?
There was a problem hiding this comment.
I think that is checked in the next line right?
since_last_warn >= self.warning_interval
This lines checks the returned timestamp drifts more than warning_threshold seconds into the future.
There was a problem hiding this comment.
Yep -- sorry, I should've spent more time reading for context.
There was a problem hiding this comment.
Sorry again for my confusion. To address your original concern: I think either behavior (comparing to self.last - now or to self.last + 1 - now) is reasonable. I had a look at at the Java implementation:
It looks like the behavior there is the same as that in the PR -- so self.last - now. Personally, I don't see a good reason to make the implementation work differently than the Java driver's. Do you object to it strongly enough?
There was a problem hiding this comment.
Yeah, self.last - now should be fine
f07576c to
b6f4a9b
Compare
|
I've changed some things about the seconds / microseconds inside the class |
da4aed3 to
2b4e111
Compare
|
@bjmb I've added a small fix that tears down the patched |
|
@mambocab great! Sure, go ahead with the squash and merge. |
5836990 to
0c25970
Compare
Addresses PYTHON-676. The history isn't great, but I plan to squash before/on merge if there are no objections.
Unit and integration tests pass locally on py2.7. I have not tested this over actual timestamp discontinuities.