We discovered an interesting race condition between Begin
and StopServer:
* Begin gets called: reserves a request, but takes time to
perform the next step, state is SERVING.
* StopService gets called: transitions state to SHUTTING_DOWN.
* StopService calls WaitForTx: the pool is empty. So, it
proceeds to perform other shutdown operations.
* Begin opens a transaction, because it was given permission.
The pool is not empty any more.
* StopService calls txpool.Close, which gets angry because
no transactions should be in it.
One possible solution I considered was to reserve an entry
in the transaction pool during startRequest. Then Begin would
later fill it out with the connection obtained after BEGIN
succeeded. This would eliminate the race, but it also
introduces weird dependencies between TabletServer and TxPool.
It would also require Begin to un-reserve if the BEGIN failed.
Overall, I felt the above approach would lead to unreadable code.
So, I went with the following approach:
* Introduce a new 'begins' waitgroup that counts number of Begin
calls in flight.
* Increment & Decrement it in startRequest and endRequest.
* Make those who want to stop query service wait for begins to
be zero before waiting on tx pool to go empty.
So, if there are any Begins in flight when we go into shut down,
we wait for them to complete. As they complete, they all reserve
a connection in the tx pool. Once the Begins are done, no new begins
are allowed, and those that completed all have created transactions.
So, at this point, it's safe to wait for tx pool to go empty.
This is not really testable. So, we have to rely on the above proof
and verification that the code was written accordingly.
EnableRowcache flag has been moved from dbconfigs.DBConfigs to
tabletserver.Config.
EnableRowcacheInvalidator has become obsolete because that decision
is dictated by tablet type. It's been deleted from dbconfigs.DBConfigs.
Removed StartService & StopService from control API.
Also made target & dbconfigs as value parameters and mandatory.
There are still a couple of plances where there is a nil check.
For now, I've changed them to IsZero check because I don't know
if something could break otherwise.
I've written a code generator that converts the python cases
into go. Some massage is needed after the conversion.
The downside is that it's a brainless port. No re-interpretation,
regrouping or gap analysis.
The go runtime schedules goroutines in any order. This means that
multiple sends can happen on a buffered channel before a runnable
receiver goroutine gets scheduled. This could cause dropped messages
even on an uncontended system.
This seems to happen very frequently for streamlog where the channel
size is 1. For now, I'm increasing this to match the original sender
channel size. Hopefully, this will lead to fewer dropped messages.
The race detector found a bug in the QueryExecutor. It probably
hasn't failed on production because the racy code paths were always
trying to set the variable to the same value.
This change also introduces constants in mysql package for flags
and types. Additionally, I mask out flag bits that are not relevant.
This will help us be deterministic about what we do or don't support.
I've also renamed queryctl.go to config.go, because what's left
is mostly Config. The Controller interface is still there. We
could move it to tabletserver.go, or leave it config.go.
For now, I haven't moved it.
queryctl was initally created because TabletServer could not
export non-rpc functions. Otherwise, the go rpc framework would
complain about those functions.
However, this has changed because we now have rpc-specific adapters.
So, this means that TabletServer can export any function it pleases.
This has made the objects in queryctl redundant.
This change pushes those thin wrappers straight into TabletServer.
Also, I've moved the mocking interface into its own package. This
prevents test code from intermixing with normal code.
There's not much left in queryctl. So, I'm currently debating if it
should remain as a file. I'm leaving it there for now.
I found a few problems and shortcomings in the mocks:
1. A lock was being held while connecting to mysql, which should
be avoided.
3. Tests depended on a global variable (DefaultDB) being initialized
to different values at the beginning of each test. This would prevent
parallelization.
2. There was no way to use a real mysql connection in tests once
a fakesqldb was setup, which is needed for the queryservice tests
to be ported to go.
In the new scheme, I've added an Engine field to ConnParams. If it's
empty, then the default (MySQL) connection is used without locks.
If there is a name, then we use the map where other test engines
are registered.
The next change will convert the memcache faking to a similar scheme.
Note the hadoop java code is broken, as it uses the old
java client and not the new one (and the bson structure changed).
The right fix is probably to switch to new client there.