Mas i1677 head #1710
Merged
Mas i1677 head #1710
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adding further unit tests demonstrated some non-determinism in the find_bestobject/1 function in that it might consider different answers to be siblings depending on the order the answers would be received. That is to say dominated siblings may or may not be included. It would be better to make this predictable, so that any riak_test that proves behaviour with siblings can't be dependent on the order of answers - increasing confidence that such situations are handled correctly
When a head is a tombstone, the get_core will convert it into something object like (using riak_object:spoof_deletedobject/1), when the response is added to the get_core. Hence we don't need to consider when running find_bestobject that head objects could be deleted. There had previously been logic in merge_heads to discount tombstone head responses as things that need to eb fetched - but they shouldn't be head-like things at that stage due to the use of spoof_deletedobjects. This logic is therefore superfluous. Also contains a check that the riak_kv_util:is_x_deleted/1 function does work as expected on HEAD-like responses.
Develop 2.9
merge_heads if it returns {fetch, IdxList} should be a list of partition IDs not of results.
Added a spec to the function to try and avoid this mistake being repeated.
Develop 2.9
Develop 2.9
|
This has been run through a standard volume test, as well as riak_test tests, as well as the extended unit tests. Without the fix the extended unit tests fail. This is not obviously critical, however, it would be safer and more reliable to have the fixes in place |
|
+1 from me. Code reviewed and discussed with Martin. Agreed including this fix in next 2.9.0 patch is appropriate. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part of the investigation into the implementation of a genuine HEAD API to Riak, the current GET code and its use of HEADs was reviewed. The outcome of this review was:
This PR adds comments and specs. It also refactors the code in attempt to both improve readability and consistency of behaviour. The unit tests have then been expanded to reflect this.
The change also removes an is_x_deleted check, as deleted tombstones (even when fetched by HEAD requests) should already not require fetching as they have already been converted into actual objects (not just HEAD objects) - https://github.com/basho/riak_kv/blob/riak_kv-2.9.0p4/src/riak_kv_get_core.erl#L109-L119.