Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-25.1: kvserver: alias range_id in flow control integration tests #139567

Open
wants to merge 7 commits into
base: release-25.1
Choose a base branch
from

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Jan 22, 2025

Backport 7/7 commits from #138085 on behalf of @kvoli.

/cc @cockroachdb/release


Earlier commits are mechanical, reformatting and deduplicating SQL queries
used by the flow control integration tests.


Previously, the range_id of scratch ranges used in various
TestFlowControl.* tests would be printed into the testdata file. This
made it tedious to introduce new system tables, which in turn may create
new ranges and necessitate regenerating the
test_flow_control_integration testdata files to pick up the higher
default scratch range id.

Alias the range id to letters in a,b,...,z so that regardless of the
underlying scratch range id changing, the aliased scratch range id
remains unchanged. For example:

An old example:

SELECT
  range_id, count(*) AS streams
FROM
  crdb_internal.kv_flow_control_handles
GROUP BY
  (range_id)
ORDER BY
  streams DESC;

  range_id | stream_count
-----------+---------------
  75       | 3
  76       | 3

Now looks like:

SELECT
  chr(96 + DENSE_RANK() OVER (ORDER BY range_id)) as range_id,
  count(*) AS streams
FROM
  crdb_internal.kv_flow_control_handles
GROUP BY
  range_id
ORDER BY
  range_id;

  range_id | stream_count
-----------+---------------
  a        | 3
  b        | 3

Resolves: #137727
Release note: None


Release justification: Test only.

kvoli and others added 7 commits December 30, 2024 15:50
These were duplicated around many tests in
`flow_control_integration_test.go`. De-duplicate.

Part of: #137727
Release note: None
The SQL query strings which are reused across several
`TestFlowControl.*` tests were inconsistently formatted. Format them
consistently, although not following any style guide.

Part of: #137727
Release note: None
Previously, the `range_id` of scratch ranges used in various
`TestFlowControl.*` tests would be printed into the testdata file. This
made it tedious to introduce new system tables, which in turn may create
new ranges and necessitate regenerating the
`test_flow_control_integration` testdata files to pick up the higher
default scratch range id.

Alias the range id to letters in `a,b,...,z` so that regardless of the
underlying scratch range id changing, the aliased scratch range id
remains unchanged. For example:

An old example:

```sql
SELECT
  range_id, count(*) AS streams
FROM
  crdb_internal.kv_flow_control_handles
GROUP BY
  (range_id)
ORDER BY
  streams DESC;

  range_id | stream_count
-----------+---------------
  75       | 3
  76       | 3
```

Now looks like:

```sql
SELECT
  chr(96 + dense_rank() OVER (ORDER BY range_id)) as range_id,
  count(*) AS streams
FROM
  crdb_internal.kv_flow_control_handles
GROUP BY
  range_id
ORDER BY
  range_id;

  range_id | stream_count
-----------+---------------
  a        | 3
  b        | 3
```

Resolves: #137727
Release note: None
These tests are now redundant, as users will run only the RACv2 protocol
post-upgrade to 24.3.

Part of: #136529
Release note: None
Epic: none
Release note: none
Previously, some `TestFlowControl.*` put requests were not printed to
the datadriven test file, only printed to the test log. This commit
updates these tests:

```
TestFlowControlRangeSplitMergeV2
TestFlowControlAdmissionPostSplitMergeV2
```

To also print to the datadriven test file on puts. The test log is also
still written to.

Epic: None
Release note: None
The following tests are updated to issue 1 byte sized puts, which are
overridden via a testing knob to be 1 MiB. Some of these tests have
previously flaked due to disconnected streams, hypothesized to be a
result of CPU starvation in the test.

```
TestFlowControlBasicV2
TestFlowControlAdmissionPostSplitMergeV2
TestFlowControlRangeSplitMergeV2
TestFlowControlBlockedAdmissionV2
TestFlowControlCrashedNodeV2
TestFlowControlRaftMembershipV2
TestFlowControlRaftMembershipRemoveSelfV2
TestFlowControlUnquiescedRangeV2
TestFlowControlClassPrioritizationV2
TestFlowControlTransferLeaseV2
TestFlowControlLeaderNotLeaseholderV2
TestFlowControlGranterAdmitOneByOneV2
```

Note, some of these tests did not have any recent failures associated
with them and the load reduction is preventative.

Informs: #137510
Informs: #138103

Release note: None
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-25.1-138085 branch from 2115309 to 77f175c Compare January 22, 2025 15:33
@blathers-crl blathers-crl bot requested a review from a team as a code owner January 22, 2025 15:33
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Jan 22, 2025
Copy link
Author

blathers-crl bot commented Jan 22, 2025

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Jan 22, 2025
Copy link
Author

blathers-crl bot commented Jan 22, 2025

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli
Copy link
Collaborator

kvoli commented Jan 22, 2025

Failure on TestFlowControlV1ToV2Transition. Going to see if this reproduces locally.

@kvoli
Copy link
Collaborator

kvoli commented Jan 23, 2025

Failure on TestFlowControlV1ToV2Transition. Going to see if this reproduces locally.

It does, in around 5 minutes of --stress --race with:

dev test pkg/kv/kvserver -v --vmodule='replica_raft=1,kvflowcontroller=2,replica_proposal_buf=1,raft_transport=2,kvflowdispatch=1,kvadmission=1,kvflowhandle=1,work_queue=1,replica_flow_control=1,tracker=1,client_raft_helpers_test=1,raft=1,admission=1,replica_flow_control=1,work_queue=1,replica_raft=1,replica_proposal_buf=1,raft_transport=2,kvadmission=1,work_queue=1,replica_flow_control=1,client_raft_helpers_test=1,range_controller=3,token_counter=2,token_tracker=2,processor=2,kvflowhandle=1' -f TestFlowControlV1ToV2Transition --stress --race --cpus=16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants