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

Add the NextSequence feature. #346

Closed
wants to merge 5 commits into from
Closed

Add the NextSequence feature. #346

wants to merge 5 commits into from

Conversation

manishrjain
Copy link
Contributor

@manishrjain manishrjain commented Dec 10, 2017

This change is Reviewable

@janardhan1993
Copy link

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


db.go, line 1104 at r1 (raw file):

// integers. Multiple sequences can be created by providing different keys. Bandwidth sets the
// size of the lease, determining how many Next() requests can be served from memory.
func (db *DB) GetSequence(key []byte, bandwidth uint64) (*Sequence, error) {

Add comment that it doesn't work in managed DB.


db.go, line 1111 at r1 (raw file):

		leased:    0,
		bandwidth: bandwidth,
	}

Throw error if bandwidth == 0


Comments from Reviewable

@deepakjois
Copy link
Contributor

  • What happens when more than one goroutine request a sequence with the same key?

  • It would nice to update the README to explain this feature. Maybe create an issue for it?


Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.


db.go, line 1108 at r1 (raw file):

		db:        db,
		key:       key,
		next:      0,

(Disclaimer: I don’t fully understand the use of this)

So, the next always starts at 0? There is no use case for values other than 0?


Comments from Reviewable

@manishrjain
Copy link
Contributor Author

  1. Same key: Adding that check would come at more complexity (tracking all the sequences in the system), something that a user can easily avoid.
  2. Added.

Review status: 1 of 5 files reviewed at latest revision, 3 unresolved discussions.


db.go, line 1104 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Add comment that it doesn't work in managed DB.

DONE. Good point. I've added a panic for mdb. And added a comment.


db.go, line 1108 at r1 (raw file):

Previously, deepakjois (Deepak Jois) wrote…

(Disclaimer: I don’t fully understand the use of this)

So, the next always starts at 0? There is no use case for values other than 0?

Yes. It's convenience v/s flexibility, I chose convenience. One can always iterate and pick values from some number N, if they want to start from N.


db.go, line 1111 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Throw error if bandwidth == 0

Done.


Comments from Reviewable

@deepakjois
Copy link
Contributor

Cool!

:lgtm_strong:


Review status: 1 of 5 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

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

Successfully merging this pull request may close these issues.

None yet

3 participants