Skip to content

Limit key/value sizes to 1G #158

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

Merged
merged 7 commits into from
Aug 14, 2017
Merged

Limit key/value sizes to 1G #158

merged 7 commits into from
Aug 14, 2017

Conversation

peterstace
Copy link

@peterstace peterstace commented Aug 11, 2017

  • Key/values can now only be 1G.
  • Checked when the user sets kv pairs.
  • Checked during value log replay, truncating if the keys are too big (since it must be from a garbage file append crash scenario).

Wasn't too sure the best way to integrate checking into the BatchSet operation. I also considered looping over the entries at the start of BatchSet, but then you have to either make multiple calls to sendToWriteCh (for each contiguous chunk of good entries), or rebuild a slice of validated entries and pass to sendToWriteCh. Neither of those options seem too nice.


This change is Reviewable

@peterstace peterstace requested a review from manishrjain August 11, 2017 04:54
@manishrjain
Copy link
Contributor

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


kv.go, line 756 at r1 (raw file):

	var bad []*Entry
	for _, entry := range entries {
		if len(entry.Key) > maxKeyValueSize || len(entry.Value) > maxKeyValueSize {

maxKeySize, maxValueSize.

1MB, 1GB.


kv.go, line 788 at r1 (raw file):

		b.Entries = bad
		b.Wg = sync.WaitGroup{}
		b.Err = ErrExceedsMaxKeyValueSize

No need to do this at request level. This should be at entry level.


kv.go, line 790 at r1 (raw file):

		b.Err = ErrExceedsMaxKeyValueSize
		b.Ptrs = nil
		reqs = append(reqs, b)

A counter here as well.


kv.go, line 793 at r1 (raw file):

	}

	return reqs

Change the way we do Set and SetAsync, and return entry level error as well.


kv.go, line 816 at r1 (raw file):

// BatchSetAsync is the asynchronous version of BatchSet. It accepts a callback function
// which is called when all the sets are complete. Any error during execution is passed as an

Not any error. Any request level error is passed as an arg. User is expected to still check errors at the Entry level.


kv.go, line 818 at r1 (raw file):

// which is called when all the sets are complete. Any error during execution is passed as an
// argument to the callback function.
func (s *KV) BatchSetAsync(entries []*Entry, f func(error)) {

18 // SetAsync is the asynchronous version of Set. It accepts a callback function which is called
17 // when the set is complete. Any error encountered during execution is passed as an argument
16 // to the callback function.
15 func (s *KV) SetAsync(key, val []byte, userMeta byte, f func(error)) {
14 e := &Entry{
13 Key: key,
12 Value: val,
11 UserMeta: userMeta,
10 }
~ 9 s.BatchSetAsync([]*Entry{e}, func(es entries, err error) {

  • 8 if err != nil {
  • 7 f(err)
  • 6 }
  • 5 for _, e := range es {
  • 4 if e.Error != nil {
  • 3 f(e.Error)
  • 2 }
  • 1 }
  • 858 f(nil)
  • 1 })
    2 }

kv.go, line 820 at r1 (raw file):

func (s *KV) BatchSetAsync(entries []*Entry, f func(error)) {
	go func() {
		err := s.BatchSet(entries)

Can't do this.


Comments from Reviewable

@peterstace
Copy link
Author

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


kv.go, line 756 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

maxKeySize, maxValueSize.

1MB, 1GB.

Done.


kv.go, line 788 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need to do this at request level. This should be at entry level.

Done.


kv.go, line 790 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

A counter here as well.

I'm not sure what you mean. Can you clarify?


kv.go, line 793 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Change the way we do Set and SetAsync, and return entry level error as well.

Done.


kv.go, line 816 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Not any error. Any request level error is passed as an arg. User is expected to still check errors at the Entry level.

Done.


kv.go, line 818 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

18 // SetAsync is the asynchronous version of Set. It accepts a callback function which is called
17 // when the set is complete. Any error encountered during execution is passed as an argument
16 // to the callback function.
15 func (s *KV) SetAsync(key, val []byte, userMeta byte, f func(error)) {
14 e := &Entry{
13 Key: key,
12 Value: val,
11 UserMeta: userMeta,
10 }
~ 9 s.BatchSetAsync([]*Entry{e}, func(es entries, err error) {

  • 8 if err != nil {
  • 7 f(err)
  • 6 }
  • 5 for _, e := range es {
  • 4 if e.Error != nil {
  • 3 f(e.Error)
  • 2 }
  • 1 }
  • 858 f(nil)
  • 1 })
    2 }

Done.


kv.go, line 820 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can't do this.

Done.


Comments from Reviewable

@peterstace
Copy link
Author

Review status: 0 of 4 files reviewed at latest revision, 7 unresolved discussions.


kv.go, line 790 at r1 (raw file):

Previously, peterstace (Peter Stace) wrote…

I'm not sure what you mean. Can you clarify?

Done.


Comments from Reviewable

@manishrjain
Copy link
Contributor

:lgtm: One comment.


Reviewed 2 of 3 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


y/metrics.go, line 48 at r3 (raw file):

	NumGets = expvar.NewInt("badger_gets_total")
	NumPuts = expvar.NewInt("badger_puts_total")
	NumBlockedLargePuts = expvar.NewInt("badger_blocked_large_puts")

badger_blocked_puts_total


Comments from Reviewable

@peterstace
Copy link
Author

Review status: 2 of 4 files reviewed at latest revision, 1 unresolved discussion.


y/metrics.go, line 48 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

badger_blocked_puts_total

Done.


Comments from Reviewable

@peterstace peterstace merged commit 3ff26e3 into master Aug 14, 2017
@deepakjois deepakjois deleted the peter/max-key-value-size branch October 30, 2017 10:29
spongedu pushed a commit to spongedu/badger that referenced this pull request Jan 14, 2021
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.

2 participants