Skip to content

Conversation

@mambocab
Copy link
Contributor

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.

@aboudreault
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line should be:
diff = self.last - now
as this value may be logged below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good catch!

since_last_warn = now - self._last_warn

warn = (self.warn_on_drift and
(diff >= self.warning_threshold) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep -- sorry, I should've spent more time reading for context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

https://github.com/datastax/java-driver/blob/da72105c2fcef8f0865047815df146cfbb2354e6/driver-core/src/main/java/com/datastax/driver/core/AbstractMonotonicTimestampGenerator.java#L60

https://github.com/datastax/java-driver/blob/da72105c2fcef8f0865047815df146cfbb2354e6/driver-core/src/main/java/com/datastax/driver/core/LoggingMonotonicTimestampGenerator.java#L65

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, self.last - now should be fine

@beltran
Copy link
Contributor

beltran commented Jan 26, 2017

I've changed some things about the seconds / microseconds inside the class MonotonicTimestampGenerator. Maybe a review would be worth it.

@beltran beltran force-pushed the timestamp-generator branch from da4aed3 to 2b4e111 Compare January 27, 2017 14:28
@mambocab
Copy link
Contributor Author

mambocab commented Feb 8, 2017

@bjmb I've added a small fix that tears down the patched time.time. I'm +1 to your changes, they look great. Unless you object, I'll squash my initial commits together, squash the last 3 together, and merge.

@beltran
Copy link
Contributor

beltran commented Feb 8, 2017

@mambocab great! Sure, go ahead with the squash and merge.

@mambocab mambocab force-pushed the timestamp-generator branch from 5836990 to 0c25970 Compare February 14, 2017 18:02
@mambocab mambocab merged commit 6c1d4f6 into apache:master Feb 14, 2017
@mambocab mambocab deleted the timestamp-generator branch February 14, 2017 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants