Skip to content
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

#797: Add sortable message timestamp #816

Merged
merged 3 commits into from
Apr 16, 2015
Merged

#797: Add sortable message timestamp #816

merged 3 commits into from
Apr 16, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Apr 10, 2015

Based on #809.

Allows subscribers of timestamping topics to sort messages.

@tseaver tseaver added the api: pubsub Issues related to the Pub/Sub API. label Apr 10, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 10, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b0209de on tseaver:797-pubsub-add_sortable_message_timestamp into * on GoogleCloudPlatform:master*.


import pytz

RFC3369 = '%Y-%m-%dT%H:%M:%S.%fZ'

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f4c51cf on tseaver:797-pubsub-add_sortable_message_timestamp into * on GoogleCloudPlatform:master*.

tseaver added 2 commits April 10, 2015 15:41
Allows subscribers of timestamping topics to sort messages.
- Ref. correct RFC.
- Private
- Add '_MICROS' to indicate which format from the sheaf allowed by the RFC.

Addresses points 1 and 2 from:
#816 (comment).

and:

#816 (comment)
@tseaver
Copy link
Contributor Author

tseaver commented Apr 10, 2015

Rebased after merge of #809.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ddb06db on tseaver:797-pubsub-add_sortable_message_timestamp into 028a15d on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor

dhermes commented Apr 10, 2015

Outside of the scope of this PR (just for my understand) what is an example of a naive timestamp being a bad idea? What problems occur?

Also note that isoformat does not produce timestamps that comply with our template

>>> import datetime
>>> import pytz
>>> now = datetime.datetime.utcnow().replace(tzinfo=pytz.UTC)
>>> now
datetime.datetime(2015, 4, 10, 21, 7, 20, 884712, tzinfo=<UTC>)
>>> now.isoformat()
'2015-04-10T21:07:20.884712+00:00'
>>> datetime.datetime(2015, 4, 10, 21, 7, 20, 0, tzinfo=pytz.UTC).isoformat()
'2015-04-10T21:07:20+00:00'
>>> datetime.datetime(2015, 4, 10, 21, 7, 20, 0).isoformat()
'2015-04-10T21:07:20'
>>> datetime.datetime(2015, 4, 10, 21, 7, 20, 100).isoformat()
'2015-04-10T21:07:20.000100'

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 04ced70 on tseaver:797-pubsub-add_sortable_message_timestamp into 028a15d on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented Apr 16, 2015

@dhermes any remaining issues?

@dhermes
Copy link
Contributor

dhermes commented Apr 16, 2015

It looks like all my issues with the code were addressed.

Though I did ask you two questions in my last message. Do you mind answering?

LGTM

@tseaver
Copy link
Contributor Author

tseaver commented Apr 16, 2015

Naive datetimes are time-bombs: when created, their zone might be one of a) UTC, b) the "local" zone on the client's machine, or c) the "local" zone on the server. Later code has no way of knowing which zone was intended, making any use of such timestamps for ordering useless. Converting from a UTC timestamp string to a naive datetime discards the information.

#818 fixes the issues with using isoformat().

tseaver added a commit that referenced this pull request Apr 16, 2015
@tseaver tseaver merged commit 2c1e1ac into googleapis:master Apr 16, 2015
@tseaver tseaver deleted the 797-pubsub-add_sortable_message_timestamp branch April 16, 2015 20:13
@dhermes
Copy link
Contributor

dhermes commented Apr 16, 2015

Right, b) and c) are the scary parts. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants