Implement global (within factory) state for alternating connections #3081
Conversation
… to make console output reflect client-side log only (less chatty)
Result of fdb-record-layer-pr on Linux CentOS 7
|
Result of fdb-record-layer-pr on Linux CentOS 7
|
| case DEFAULT: | ||
| return DEFAULT_CONNECTION; | ||
| case ALTERNATE: | ||
| return currentConnection.addAndGet(1) % (1 + alternateFactories.size()); |
There was a problem hiding this comment.
This doesn't look right. Why are you doing 1+ here. If you have 2 connections, and initialConnection is 0 I think this would return:
- 0 + 1 % 3 -> 1
- 1 + 1 % 3 -> 2
- 2 + 1 % 3 -> 0
- 3 + 1 % 3 -> 1
But 2 is not valid.
And returning 1 seems wrong.
But, if my reading is right, this would fail tests, so maybe I'm wrong.
It might be clearer with
currentConnection.getAndUpdate(i -> (i + 1) % alternateFactories.size());
There was a problem hiding this comment.
the 1 is to account for the primary connection. Normally, we would have 1 defaultFactory and 1 alternateFactory in the list.
It is incrementing prematurely, though, so the right statement is:
currentConnection.getAndAdd(1) % (1 + alternateFactories.size())
or
currentConnection.getAndUpdate(i -> (i + 1) % (1 + alternateFactories.size()))
There was a problem hiding this comment.
The difference is that the % is either accounted for in the counter or after the fact - I thought it is nice that it actually count the entire number of connections created, but don't have strong opinion on that
There was a problem hiding this comment.
Added field for the total number of connections, hopefully this will make it easier to follow.
Changed the logic to modulo the current connection to be within the range of the number of factories
There was a problem hiding this comment.
Oh, yeah, that is where I was confused, the fact that there is the "default" and the alternate.
| classUnderTest.getNewConnection(URI.create("Blah")); | ||
| assertEquals(ic + 1, classUnderTest.getCurrentConnection()); | ||
| classUnderTest.getNewConnection(URI.create("Blah")); | ||
| assertEquals(ic + 2, classUnderTest.getCurrentConnection()); |
There was a problem hiding this comment.
I wonder whether it would make sense to try and have a more integrated test, similar to SupportedVersionTest, where the test would fail if it did anything other than alternate.
There was a problem hiding this comment.
Not sure what you mean. Does the added coverage meet that?
There was a problem hiding this comment.
I was thinking something that has an actual .yamsql file, and would fail if it did anything other than alternate connections. But I think that would be a lot of work, and not a lot of additional coverage.
There was a problem hiding this comment.
I see. That's a good idea, but like you said, may take some time. I'll keep it around for future improvements?
- Change logic to have selector reflect the next connection returned - Modulo the connection number instead of a counter - Add constant for total number of connections - Add tests for actual alternating of initial connections - limit initial connection to pool size
Result of fdb-record-layer-pr on Linux CentOS 7
|
| case DEFAULT: | ||
| return DEFAULT_CONNECTION; | ||
| case ALTERNATE: | ||
| return currentConnection.addAndGet(1) % (1 + alternateFactories.size()); |
There was a problem hiding this comment.
Oh, yeah, that is where I was confused, the fact that there is the "default" and the alternate.
| classUnderTest.getNewConnection(URI.create("Blah")); | ||
| assertEquals(ic + 1, classUnderTest.getCurrentConnection()); | ||
| classUnderTest.getNewConnection(URI.create("Blah")); | ||
| assertEquals(ic + 2, classUnderTest.getCurrentConnection()); |
There was a problem hiding this comment.
I was thinking something that has an actual .yamsql file, and would fail if it did anything other than alternate connections. But I think that would be a lot of work, and not a lot of additional coverage.
ScottDugas
changed the title
This fixes an issue with the connection factory where YAML tests with a single statement always ended up using the same connection number.