Skip to content

Add the NextSequence feature. #346

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

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

Successfully merging this pull request may close these issues.

3 participants