server: move sqlServer to new file #47509
Merged
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
This is pure code movement. Next step will be untangling `(*Server).Start` to contain an explicit call to `(*sqlServer).start()` (a method which does not exist today). I'm intentionally not documenting the various inputs to `sqlServer` yet in anticipation of larger changes to them. Release note: None
|
This change is |
andreimatei
reviewed
Apr 15, 2020
There was a problem hiding this comment.
LGTM
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)
|
bors r=andreimatei |
Build succeeded |
craig bot
pushed a commit
that referenced
this pull request
Apr 16, 2020
47331: storage_test: test incremental iteration via ExportToSST r=dt a=dt TestMVCCIncrementalIterator used to serve as one of our primary unit tests on incremental iteration. However with the introduction of the c++ implementation of an incremental iterator, the fact that it directly tests the Go iterator leaves the parallel implementation (that is used by default at runtime) untested. This change extends the test to also verify each test case via engine.ExportToSST, which, because it lets the engine in use decide which iterator to use, means that it now tests both iterators. Additionally this serves to actually test that the usage of those iterators yields the expected results at the same time. Futhermore, both of these iterators behave differently when the min/max timestamp hints are provided, when they use a second filtered (time-bound) iterator to skip keys. Previously this test never set these hints, and thus never exercise the TBI path in either iterator. Not however that even with this hint set and the pair of iterators being used, this test still only covers a fraction of the TBI code, as the handful of keys used in each test never get flushed, much less into separate SSTs with different time-bounds, so all of the logic around Seeking over large swaths of keys, catching up in the known edges cases, etc is still uncovered. Release note: none. 47517: server: extract (*sqlServer).start r=andreimatei a=tbg First commit is #47509, don't review here. Co-authored-by: David Taylor <[email protected]> Co-authored-by: Tobias Schottdorf <[email protected]>
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Apr 20, 2020
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Apr 22, 2020
craig bot
pushed a commit
that referenced
this pull request
Apr 22, 2020
47616: kv/concurrency: remove TODO about impossible deadlock scenario r=nvanbenschoten a=nvanbenschoten This commit removes a TODO that described a potential scenario in which a request-only dependency cycle could deadlock due to a simultaneous transaction abort. I realized after writing a full fix (9a9182f) that such a deadlock was not achievable in practice due to the semantics of the lockTable. > It may appear that there is a bug here in the handling of request-only > dependency cycles. If such a cycle was broken by simultaneously aborting > the transactions responsible for each of the request, there would be no > guarantee that an aborted pusher would notice that its own transaction > was aborted before it notices that its pushee's transaction was aborted. > For example, in the simplest case, imagine two requests deadlocked on > each other. If their transactions are both aborted and each push notices > the pushee is aborted first, they will both return here triumphantly and > wait for the other to exit its lock wait-queues, leading to deadlock. > Even if they eventually pushed each other again, there would be no > guarantee that the same thing wouldn't happen. > > However, such a situation is not possible in practice because such a > dependency cycle is never constructed by the lockTable. The lockTable > assigns each request a monotonically increasing sequence number upon its > initial entrance to the lockTable. This sequence number is used to > straighten out dependency chains of requests such that a request only > waits on conflicting requests with lower sequence numbers than its own > sequence number. This behavior guarantees that request-only dependency > cycles are never constructed by the lockTable. Put differently, all > dependency cycles must include at least one dependency on a lock and, > therefore, one call to pushLockTxn. Unlike pushRequestTxn, pushLockTxn > actively removes the conflicting lock and removes the dependency when it > determines that its pushee transaction is aborted. This means that the > call to pushLockTxn will continue to make forward progress in the case of > a simultaneous abort of all transactions behind the members of the cycle, > preventing such a hypothesized deadlock from ever materializing. The PR also includes a good amount of other cleanup that I was intending to land with the fix to this deadlock. For instance, it splits off a `LockAcquisition` type from the `LockUpdate` type. It also cleans up the internals of `lock_table.go`. 47727: server: various server construction cleanup r=nvanbenschoten a=nvanbenschoten This is all cleanup I stumbled upon while catching up on the changes made in the following PRs: - #46843 - #46975 - #47509 - #47517 - #47570 It's all pretty minor. Co-authored-by: Nathan VanBenschoten <[email protected]>
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.
This is pure code movement. Next step will be untangling
(*Server).Startto contain an explicit call to(*sqlServer).start()(a method which does not exist today).
I'm intentionally not documenting the various inputs to
sqlServeryetin anticipation of larger changes to them.
Release note: None