Make default and persistent options specifyable via annotations to fdb.options... #1767

ajbeamon · 2019-06-28T18:20:15Z

Fix some issues with persisting these options in the multi-version client. Make size limit option not persistent.

ajbeamon · 2019-06-28T18:24:57Z

ajbeamon · 2019-06-28T18:47:09Z

I have two tests to try which I haven't yet -- that this works as expected when connecting to a 6.1 cluster through the multi-version client and that the behavior works correctly during version switches.

alecgrieser

I think this mostly looks good. It might also require a release note (or something) that one should use the 6.2 client as the primary client in multi-version configurations in order to properly handle these things unless the 6.1 client works (but I think it would have problems).

bindings/python/tests/size_limit_tests.py

fdbclient/FDBOptions.h

alecgrieser · 2019-06-28T18:54:08Z

fdbclient/MultiVersionTransaction.actor.cpp

+		}
+
+		if(timeout.present()) {
+			newTr.transaction->setOption(FDBTransactionOptions::TIMEOUT, timeout);


Why is timeout handled separately like this?

I think it's the case that setting the timeout can immediately end a transaction if the timeout has elapsed. This can cause a problem if, for example, you set a database default at 1 second and then override it to 5 seconds on the transaction. If the transaction has run for 2 seconds and you apply both timeouts, the database default will then cause you to fail.

The special logic here is to make sure we only set the most recent value of the timeout.

I can add a comment to this effect.

Hm, wait, if persistentOptions were a UniqueOrderedOptionList, would that solve this? Or is there some reason it can't be one of those?

That would do it, and I could be convinced to go that route for persistent options. The reason I didn't was because it also imposes the constraint that persistent options cannot have cumulative effects (like database defaults), and I made the choice to not do that since this workaround was easy enough.

It's not a position I hold strongly, though, so feel free to change my mind.

None of the current persistent options have cumulative effects, right? So, if this went in as is, we could change it to use the UniqueOrderedOptionList later? Maybe we should keep it, then, as it doesn't restrict our options.

Yes, I believe that to be the case.

fdbclient/MultiVersionTransaction.actor.cpp

fdbclient/ReadYourWrites.actor.cpp

fdbclient/vexillographer/fdb.options

…b.options. Fix some issues with persisting these options in the multi-version client. Make size limit option not persistent.

AlvinMooreSr · 2019-06-29T13:53:58Z

@fdb-build test this please

fdbclient/ReadYourWrites.actor.cpp

fdbclient/MultiVersionTransaction.actor.cpp

jzhou77 · 2019-07-02T17:00:12Z

fdbclient/NativeAPI.actor.cpp

 		}
 		catch (Error& e) {
-			retryCount++;
-			if (retryCount == 10)


Where do we handle the retry logic now? At some transaction class now?

Oh good catch, this was from me trying to delete something else. I'll add this logic back.

Use emplace_back instead of push_back Co-Authored-By: Jingyu Zhou <[email protected]>

jzhou77 · 2019-07-02T20:40:06Z

fdbclient/vexillographer/vexillographer.cs

-        private string _comment;
+        public bool persistent { get; set; }
+        public int defaultFor { get; set; }
+		private string _comment;


Use space to be consistent.

jzhou77 · 2019-07-02T20:40:52Z

fdbclient/vexillographer/vexillographer.cs

+                        bool persistent = oDoc.AttributeOrNull("persistent") == "true";
+                        String defaultForString = oDoc.AttributeOrNull("defaultFor");
+						int defaultFor = defaultForString == null ? -1 : int.Parse(defaultForString);
+						string disableOn = oDoc.AttributeOrNull("disableOn");


For these two lines, use space to be consistent.

jzhou77

LGTM. There are a couple of space formatting comments inlined.

alecgrieser · 2019-07-02T22:24:35Z

Looks like the macOS build is complaining about missing options in a switch statement:

fdbclient/NativeAPI.actor.cpp:753:10: error: 4 enumeration values not handled in switch: 'TRANSACTION_TIMEOUT', 'TRANSACTION_RETRY_LIMIT', 'TRANSACTION_MAX_RETRY_DELAY'... [-Werror,-Wswitch]
                switch(option) {

Those are the database-wide options, so they are being handled, just not the way it thinks. I'm not sure if a default statement just to put a comment is the right thing or not.

…ing options (and logging for present options).

ajbeamon · 2019-07-03T01:16:09Z

Ok, I've run the binding tester on this against a 6.1 cluster using libfdb_c.so from this branch as the primary library, and it worked as expected.

I've also run a few tests of timeouts during upgrades, and they all worked as expected. I tested a timeout set as a database default, as a transaction option (set once at the beginning), and setting both, with the default short and the transaction timeout long. In all cases, the timeouts fired the expected amount of time after the upgrade.

It is the case that any state about the current elapsed time or retry count is lost, but I'm not sure whether we want to go the route of trying to address that. I don't think it's within scope of this PR, at any rate.

…ient must be used as the primary for the behavior to work correctly.

ajbeamon · 2019-07-09T22:26:49Z

I've updated the release notes, as suggested. I don't think there are any other requested changes, can you confirm and merge if it looks good?

ajbeamon · 2019-07-09T22:27:23Z

Well, I guess I need to merge master first and resolve some conflicts.

# Conflicts: # documentation/sphinx/source/release-notes.rst

alecgrieser · 2019-07-09T23:23:05Z

Well, looks good modulo the failing build.

ajbeamon · 2019-07-10T01:27:24Z

Looking at one of the checks, it looks like there are disk space issues.

alecgrieser · 2019-07-10T01:29:14Z

Yeah, I think these are the things Alex said he resolved by freeing up some space.

I'd say we just run t*st this please except for the new conflict in NativeAPI.actor.cpp.

# Conflicts: # fdbclient/NativeAPI.actor.cpp

ajbeamon · 2019-07-10T15:00:22Z

Checks have all passed now

ajbeamon requested review from alecgrieser and jzhou77 June 28, 2019 18:45

ajbeamon assigned alecgrieser Jun 28, 2019

alecgrieser requested changes Jun 28, 2019

View reviewed changes

ajbeamon added 2 commits June 28, 2019 13:24

Make default and persistent options specifyable via annotations to fd…

2035b36

…b.options. Fix some issues with persisting these options in the multi-version client. Make size limit option not persistent.

Move size_limit.py to size_limit_tests.py

c6df30d

ajbeamon force-pushed the fix-mvc-default-options branch from be19eeb to c6df30d Compare June 28, 2019 20:25

Address some review comments

aa1bc00

jzhou77 reviewed Jul 2, 2019

View reviewed changes

fdbclient/ReadYourWrites.actor.cpp Outdated Show resolved Hide resolved

jzhou77 reviewed Jul 2, 2019

View reviewed changes

fdbclient/MultiVersionTransaction.actor.cpp Outdated Show resolved Hide resolved

jzhou77 reviewed Jul 2, 2019

View reviewed changes

ajbeamon and others added 4 commits July 2, 2019 11:09

Apply suggestions from code review

7e5b5a0

Use emplace_back instead of push_back Co-Authored-By: Jingyu Zhou <[email protected]>

Restore retry limiting on the client sampling transaction

e7218bb

Undo formatting changes to otherwise unchanged code

1cf449d

Remove space

3f6ba3d

jzhou77 reviewed Jul 2, 2019

View reviewed changes

jzhou77 approved these changes Jul 2, 2019

View reviewed changes

Fix whitespace

c9ed860

Add a break to setOption() switch statement. Better detection of miss…

6b6012e

…ing options (and logging for present options).

jzhou77 mentioned this pull request Jul 8, 2019

Add transaction getApproximateSize() API #1756

Merged

Add a release note for option fix, in particular noting that a 6.2 cl…

c8cf7f8

…ient must be used as the primary for the behavior to work correctly.

Merge branch 'master' into fix-mvc-default-options

15ecba5

# Conflicts: # documentation/sphinx/source/release-notes.rst

Merge branch 'master' into fix-mvc-default-options

a174178

# Conflicts: # fdbclient/NativeAPI.actor.cpp

alecgrieser approved these changes Jul 10, 2019

View reviewed changes

alecgrieser merged commit a72d5b5 into apple:master Jul 10, 2019

ajbeamon deleted the fix-mvc-default-options branch July 10, 2019 21:45

Make default and persistent options specifyable via annotations to fdb.options... #1767

Make default and persistent options specifyable via annotations to fdb.options... #1767

Uh oh!

Conversation

ajbeamon commented Jun 28, 2019

Uh oh!

ajbeamon commented Jun 28, 2019

Uh oh!

ajbeamon commented Jun 28, 2019

Uh oh!

alecgrieser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlvinMooreSr commented Jun 29, 2019

Uh oh!

Uh oh!

Uh oh!

jzhou77 Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzhou77 left a comment

Choose a reason for hiding this comment

Uh oh!

alecgrieser commented Jul 2, 2019

Uh oh!

ajbeamon commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajbeamon commented Jul 9, 2019

Uh oh!

ajbeamon commented Jul 9, 2019

Uh oh!

alecgrieser commented Jul 9, 2019

Uh oh!

ajbeamon commented Jul 10, 2019

Uh oh!

alecgrieser commented Jul 10, 2019

Uh oh!

ajbeamon commented Jul 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jzhou77 Jul 2, 2019 •

edited

Loading

ajbeamon commented Jul 3, 2019 •

edited

Loading