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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose APPEND_IF_FITS to clients #22

Merged
merged 3 commits into from Mar 7, 2018

Conversation

Projects
None yet
2 participants
@alecgrieser
Contributor

alecgrieser commented Mar 3, 2018

We already had an atomic mutation, APPEND_IF_FITS, that was recognized by the server insofar as it would apply mutations of that type. It was only exposed to users who were using the NativeAPI, which meant that none of the bindings could make use of it. (We even had some 馃憣logic throwing invalid client operations if anyone even dared try, which was fun to discover while debugging, I'll let you know.)

This operation is a little bit spicier than your average atomic mutation in that a na茂ve user might find themselves in the situation where they lose data because they append to a key without reading it and, oops, exceed the maximum value size (100 kB, but tweakable with knobs). I included a warning message telling people as such, but it takes a pretty scary warning message to convince people not to do things. One thing we could do is make people set some kind of transaction option (like tr.options.setEnableAppendIfFitsBecauseImSureIWontEverExceedTheMaximimValueSize or something) and only let it be used on transactions like that. But maybe that's overhead, and I haven't done it here.

Also, 6 pm Friday PRs are the best PRs.

@alecgrieser alecgrieser requested review from bnamasivayam and ajbeamon Mar 3, 2018

@alecgrieser alecgrieser changed the title from Expose APPEND_IF_FITS as to clients to Expose APPEND_IF_FITS to clients Mar 3, 2018

@ajbeamon

This looks good to me. We should confirm with the team that we are happy to reintroduce this feature and then let's merge it in.

@alecgrieser

This comment has been minimized.

Contributor

alecgrieser commented Mar 5, 2018

Yeah, that sounds reasonable.

@alecgrieser

This comment has been minimized.

Contributor

alecgrieser commented Mar 7, 2018

We talked to the team, and they seemed okay with it, so IN IT GOES!

@alecgrieser alecgrieser merged commit 2a2ac56 into apple:release-5.2 Mar 7, 2018

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