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 warning/max object limits #680

Merged
merged 1 commit into from
Oct 17, 2013
Merged

Conversation

engelsanchez
Copy link
Contributor

CSEs have wanted more visibility into sibling and object size explosions for a while. A warning threshold would make sure that the logs can be used to pinpoint or event prevent size and sibling number explosions.

We have also discussed putting hard limits on size and sibling number. Writes would fail for very large objects or objects with too many siblings. This should only be used as a last resort to prevent a cluster from experiencing severe performance problems that can result in cascaded deaths even

@ghost ghost assigned engelsanchez Oct 2, 2013
Reading or writing objects whose binary representation is bigger than
warn_object_size will generate a warning in the logs identifying the
bucket/key and size for the object.

Putting an object whose binary size is greater than max_object_size will
fail.

We will now warn if writing more than warn_siblings and fail to put if
more than max_siblings.
@engelsanchez
Copy link
Contributor Author

Configuration schema changes for this are in basho/riak#397 and tests in basho/riak_test#398

@@ -1701,13 +1711,55 @@ return_encoded_binary_object(Method, EncodedObject) ->
{{error, Reason::term(), UpdModState::term()}, EncodedObj::binary()}.

encode_and_put(Obj, Mod, Bucket, Key, IndexSpecs, ModState) ->
NumSiblings = riak_object:value_count(Obj),
case NumSiblings > app_helper:get_env(riak_kv, max_siblings) of
Copy link
Contributor

Choose a reason for hiding this comment

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

should we handle the case that someone has set the number to some non_integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any non integer term is larger than any integer in Erlang. So it handles itself: it would behave as if there wasn't a threshold

@evanmcc
Copy link
Contributor

evanmcc commented Oct 16, 2013

A potential optimization here would be to stuff all of these values into a 4-tuple and have a function to unpack them. As yet I have no idea if all of the extra ets calls are going to be an issue.

Code looks good, on to some testing.

@engelsanchez
Copy link
Contributor Author

You would be surprised by how efficient these ETS lookups are when measured with DTrace in the general case. Still, I added them as is because I have some plans to optimize configuration that I will tell you about elsewhere. It would be a post-ricon thing and would benefit from keeping plain app_helper:get_env calls instead of trying to optimize by hand. I have a todo item on that.

@evanmcc
Copy link
Contributor

evanmcc commented Oct 16, 2013

No noticeable performance impacts in its current form, code seems sensible and does what is described.
👍 Additional comments on the default settings have been left on the appropriate PR.

engelsanchez added a commit that referenced this pull request Oct 17, 2013
@engelsanchez engelsanchez merged commit 0af1d2f into develop Oct 17, 2013
@engelsanchez engelsanchez deleted the feature/object-limits branch October 17, 2013 15:43
@rzezeski rzezeski modified the milestones: 2.0-beta, 2.0 Mar 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants