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

Feature/pn 310 ttl for messages #7

Merged
merged 16 commits into from
Mar 21, 2017
Merged

Conversation

bogh
Copy link
Collaborator

@bogh bogh commented Mar 17, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 66.394% when pulling 4a3cd88 on feature/pn-310-ttl-for-messages into f81bfe6 on master.

ID: id,
Path: Path(meta[0]),
UserID: meta[2],
ApplicationID: meta[3],
Expires: expiresTime,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This field should be ExpirationTime not Expires.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 67.29% when pulling 8f6ad3f on feature/pn-310-ttl-for-messages into 8b0fccb on master.

// consider the message as processed and log the action
//
// RFC3339 format
Expires *time.Time
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you using RFC3339 for Expires, but a unix timestamp for Time below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We require the message to also contain timezone information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for me asking, but why are you requiring the timezone information? Wouldn't it suffice to know the exact time the message expires? Which is given by a unix timestamp (since it is in UTC).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You might be correct, but we had some issues with timezone information checking on other project and would like for the producer to be able to fully control without any uncertainty.

@bogh bogh mentioned this pull request Mar 21, 2017
Copy link
Owner

@cosminrentea cosminrentea left a comment

Choose a reason for hiding this comment

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

after seeing the comments, would prefer unix timestamp instead of a string format -> but in a another pull-request

@bogh bogh merged commit a74bed7 into master Mar 21, 2017
@bogh bogh deleted the feature/pn-310-ttl-for-messages branch March 21, 2017 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants