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

Use timestamp to communicate collection_timestamp of the parent #1770

Merged
merged 5 commits into from
Sep 20, 2018

Conversation

glasserc
Copy link
Contributor

Don't re-use old self.timestamp when notifying of event. Instead,
re-compute it fresh.

Fixes #1769.

  • Add documentation.
  • Add tests.
  • Add a changelog entry.
  • (n/a) Add your name in the contributors file.
  • (n/a) If you changed the HTTP API, update the API_VERSION constant and add an API changelog entry in the docs
  • (n/a) If you added a new configuration setting, update the kinto.tpl file with it.

Don't re-use old self.timestamp when notifying of event. Instead,
re-compute it fresh.

Fixes Kinto#1769.
@glasserc
Copy link
Contributor Author

Another approach would be to turn the @reify'd timestamp property into a @property. As far as I can tell, the @reify is a performance optimization -- fewer database round trips. Taking it out doesn't seem to break any tests (except for a few that rely on the ability to set the timestamp of a resource).

Running the test suite with @reify on my laptop finishes in 225-235 seconds. Running with @property finishes in 240-255. So I guess we should probably keep the @reify.

@leplatrem suggested that I add a comment to this change explaining it, which I'll do.

@glasserc glasserc force-pushed the use-up-to-date-timestamp branch from 1ab7209 to 2ec091b Compare September 20, 2018 15:09
@glasserc
Copy link
Contributor Author

I just added one test (for put) in a section where tests are also present for other verbs. What do you think about this, @leplatrem ? Is this test even in the right place?

Copy link
Contributor

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

That looks correct to me ;)

@leplatrem
Copy link
Contributor

Running the test suite with @reify on my laptop finishes in 225-235 seconds. Running with @Property finishes in 240-255. So I guess we should probably keep the @reify.

I'm not sure it's a clear indicator, since most of the test use the memory backend. Some load test maybe more accurate, but again with PG on the same machine I don't know if the difference will be huge.

@leplatrem
Copy link
Contributor

@glasserc
Copy link
Contributor Author

glasserc commented Sep 20, 2018

OK. I am going to merge just this smaller fix, knowing that perhaps it isn't the best long-term solution, but right now I don't want to untangle the significance of the timestamp property being reifyd. Thanks!

@glasserc glasserc merged commit 5dc523b into Kinto:master Sep 20, 2018
@glasserc glasserc deleted the use-up-to-date-timestamp branch September 20, 2018 22:25
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.

2 participants