Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
75499: sql: add constraint IDs and use them for comments r=fqazi a=fqazi This pull request will make the following changes: 1) Modify table descriptor logic to start allocating constraint IDs for each table constraint. 2) Add a migration to upgrade existing descriptors to have constraint IDs 3) Modify the comment updater / comment logic to references constraints by ConstraintID and update the obj_description builtin to look these up. 75888: kvstreamer: fix a bug with incorrect processing of empty scan responses r=yuzefovich a=yuzefovich **kvstreamer: fix a bug with incorrect processing of empty scan responses** Previously, the `Streamer` could get stuck indefinitely when a response for a Scan request came back empty - namely the response neither had no bytes inside nor ResumeSpan set (this is the case when there is no data in the key span to scan). This would lead to `GetResults` call being stuck thinking there are more responses to come back, and none would show up. This is now fixed by creating a Result with an empty Scan response inside of it. This approach makes it easier to support Scans that span multiple ranges and the last range has no data in it - we want to be able to set Complete field on such an empty Result. Since this was the only known bug with the `Streamer` implementation, the streamer is now used by default again. Fixes: #75708. Release note: None **kvstreamer: minor cleanup** This commit replaces the last buffered channel we had in the `Streamer` code in favor of a condition variable which simplifies the control flow a bit. Additionally, this commit refactors the code so that empty Get responses are not returned which allows us to clean up `TxnKVStreamer` a bit. Care has to be taken so that we still signal the client's goroutine waiting for results when an empty Get response comes back (similar to the bug fixed by the previous commit). Also, the tracking of the "number of outstanding requests" in `TxnKVStreamer` has been removed since it provided little value (and probably was broken anyway). Release note: None 76189: dev: test using datadriven r=irfansharif a=irfansharif This commit groups a few changes to speed up dev and make it easier to test. 1. Update `dev bench` to run benchmarks using `bazel test` directly instead of first grepping for benchmarks, querying test targets, and only then invoking test binaries using `bazel run`. The latter approach was both slower and more difficult to test. `dev bench` now looks identical to the `dev test` implementation. 2. Simplify `dev test`; now that protobuf targets are directly buildable, we don't need any manual querying of build targets and filtering for only go_test ones -- we can more easy `bazel test` whatever was top-level package was specified. This reduces how much I/O needs to happen with the host environment which both speeds things up and makes it easier to test. a. It also allows us to partially revert [#74808](#74808) while still retaining informative error messages about missing targets (bazel's ones out-of-the-box are pretty good). 3. Finally, introduce TestDataDriven and TestRecorderDriven. a. TestDataDriven makes use of datadriven to capture all operations executed by individual dev invocations. The testcases are defined under testdata/datadriven/*. DataDriven divvies up these files as subtests, so individual "files" are runnable through: dev test pkg/cmd/dev -f TestDataDrivenDriven/<fname> [--rewrite] OR go test ./pkg/cmd/dev -run TestDataDrivenDriven/<fname> [-rewrite] b. TestRecorderDriven makes use of datadriven/recorder to record (if --rewrite is specified) or play back (if --rewrite is omitted) all operations executed by individual dev invocations. The testcases are defined under testdata/recorderdriven/\*; each test file corresponds to a captured recording found in testdata/recorderdriven/\*.rec. dev test pkg/cmd/dev -f TestRecorderDriven/<fname> OR go test ./pkg/cmd/dev -run TestRecorderDriven/<fname> [-rewrite] Recordings are used to mock out "system" behavior. When --rewrite is specified, attempts to shell out to bazel or perform other OS operations (like creating, removing, symlinking filepaths) are intercepted and system responses are recorded for future playback. It's worth comparing TestDataDriven and TestRecorderDriven. a. TestDataDriven is well suited for exercising flows that don't depend on reading external state in order to function (simply translating a `dev test <target>` to its corresponding bazel invocation for e.g.) All operations are run in "dry-run" mode when --rewrite is specified; all exec/os commands return successfully with no error + an empty response, and just the commands are recorded as test data. b. TestRecorderDriven in contrast works better for flows that do depend on external state during execution (like reading the set of targets from a file for e.g., or hoisting files from a sandbox by searching through the file system directly). With these, when --rewrite is specified, we actually do shell out and record the data for future playback. We previously (#68514) ripped out the equivalent of TestRecorderDriven because it was difficult to use (large recordings, execution errors failing the test, etc.) -- issues this PR tries to address. There are some real limitations here though that may still make this approach untenable: When --rewrite is used for TestRecorderDriven, because it shells out, will: - takes time proportional to the actual dev invocations; - makes it difficult to run under bazel (through without --rewrite it works just fine); The few remaining tests for this recorder stuff are probably examples of dev doing too much, when it should instead push the logic down into bazel rules. As we continue doing so, we should re-evaluate whether this harness provides much value. Release note: None Co-authored-by: Faizan Qazi <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: irfan sharif <[email protected]>
- Loading branch information
