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

Fix documentation of Expiry #84

Closed
wants to merge 1 commit into from
Closed

Fix documentation of Expiry #84

wants to merge 1 commit into from

Conversation

runerei
Copy link

@runerei runerei commented May 30, 2018

@cb-sdk-robot
Copy link

Thanks for the pull request!! To ensure quality review, Couchbase employs a code review system based on Gerrit to manage the workflow of changes in addition to tracking our contributor agreements.

To get this change in and collaborate in code review, please register on Gerrit and accept our CLA. The easiest way to do this is to follow the link below, sign in with your GitHub account and then follow through the steps provided on that page to sign an 'Individual' agreement: http://review.couchbase.org/#/settings/new-agreement.

Keep in mind that the emails we are seeing on the commits are: r**e@pa***no

Note: Please contact us if you have any issues registering with Gerrit! If you have not signed our CLA within 7 days, the Pull Request will be automatically closed.

::SDKBOT/PR:no_cla

@MikeGoldsmith
Copy link
Contributor

Hi @runerei

Thanks for the PR. However, the Document.Expiry property is expressed as milliseconds, not seconds. You can see it in action here (note: this is a Memcached bucket test, but applies to Couchbase buckets too).

Also, I'm unsure why you think the link you provided indicates timeouts are mean to be in seconds, the linked page does not describe the expiry precision and the .NET example shows it be used with milliseconds.

If my assumptions are incorrect, or I've misunderstood, please let me know.

Thanks.

@cb-sdk-robot
Copy link

Unfortunately it has been 7 days and we are still unable to confirm that you have signed our CLA. We sincerely appreciate your submission and hope that you will register and resubmit this Pull Request in the future!

::SDKBOT/PR:timeout

@runerei
Copy link
Author

runerei commented Jun 20, 2018

Sorry if I'm wrong. From this code it looked like it was using seconds.

public static uint ToTtl(this TimeSpan duration)

However there are some string parsing in the same class implying support for different time resolutions.

What I found was that if the expiry is in milliseconds it will overflow after 50 days.

Does it use milliseconds when expiry is less than 30 days and seconds when it is more?

@MikeGoldsmith
Copy link
Contributor

Hmm, it looks like there is some confusion regarding these values.

I'll look into it further and report back. I've created a JIRA ticket NCBC-1714 to track it. Thanks for the report and sorry for the confusion.

@runerei
Copy link
Author

runerei commented Jun 26, 2018

As a comment to NCBC-1714, using uint and not TimeSpan how will you express expiry of

  • 10 seconds
  • 10 months

And what is the expected expiration value of the raw Couchbase document?

By the way, in this example node.js is using expiry in seconds.

@MikeGoldsmith
Copy link
Contributor

Hi @runerei

I finally got some time to look into this properly and think I've found the points have have been confusing us both;

  • document expiries are always expressed as seconds if the value is less 30 days or a unix timestamps if greater on the server - link
  • the .NET API exposes three ways to control the expiry (not ideal):
    • directly in the API via a TimeSpan or uint parameter (seconds)
    • As a uint on the Document class (expressed as milliseconds)

This is where the confusion comes in; we use an internal function ToTtl() to convert either TimeSpan or uint into the expiry value expressed as seconds.

Likely Document.Expiry should not be a uint, and should be a TimeSpan for consistentcy - same for removing the unit variant on the bucket API. For now, we can't change that as it would be a breaking change, hopefully we can do better in the 3.0 API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants