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

multitenant: fix tenant_upgrade_test to allow new 23.2 versions to be added #102030

Merged
merged 1 commit into from
Apr 21, 2023
Merged

multitenant: fix tenant_upgrade_test to allow new 23.2 versions to be added #102030

merged 1 commit into from
Apr 21, 2023

Conversation

ecwall
Copy link
Contributor

@ecwall ecwall commented Apr 21, 2023

Fixes #102028

Previously adding a new version would break this test

Failed
=== RUN   TestTenantUpgradeFailure/upgrade_tenant_have_it_crash_then_resume
    tenant_upgrade_test.go:352: query 'SHOW CLUSTER SETTING version': expected:
        1000023.1

        got:
        1000023.1-2

    --- FAIL: TestTenantUpgradeFailure/upgrade_tenant_have_it_crash_then_resume (60.42s)

Now the -* part of the version is ignored.

Release note: None

… added

Fixes #102028

Previously adding a new version would break this test
```
Failed
=== RUN   TestTenantUpgradeFailure/upgrade_tenant_have_it_crash_then_resume
    tenant_upgrade_test.go:352: query 'SHOW CLUSTER SETTING version': expected:
        1000023.1

        got:
        1000023.1-2

    --- FAIL: TestTenantUpgradeFailure/upgrade_tenant_have_it_crash_then_resume (60.42s)
```

Now the `-*` part of the version is ignored.

Release note: None
@ecwall ecwall requested a review from knz April 21, 2023 20:07
@ecwall ecwall requested a review from a team as a code owner April 21, 2023 20:07
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ecwall
Copy link
Contributor Author

ecwall commented Apr 21, 2023

Tested this here #101869

@ecwall
Copy link
Contributor Author

ecwall commented Apr 21, 2023

bors r=healthy-pod

@craig
Copy link
Contributor

craig bot commented Apr 21, 2023

Build succeeded:

@craig craig bot merged commit 23f5264 into cockroachdb:master Apr 21, 2023
craig bot pushed a commit that referenced this pull request Apr 26, 2023
97728: lease: Fixed a small if-condition for AcquireFreshestFromStore r=Xiang-Gu a=Xiang-Gu

Previously, the condition is `if xx > 1` while it's enough to do `if xx > 0`.

Epic: None

101869: sql: implement DESCENDING order PKs for row-level TTL r=rafiss a=ecwall

Fixes #76912

Updates the schema change validation to allow TTL tables to have DESC PK columns. The functionality is version gated behind `V23_2TTLAllowDescPK`.

Changes the TTL query bounds `WHERE` clause from this format:
```
AND (col1, col2) >= ($4, $5) AND (col1, col2) < ($2, $3)
```
to this format:
```
AND (
  (col1 > $4) OR
  (col1 = $4 AND col2 >= $5)
)
AND (
  (col1 < $2) OR
  (col1 = $2 AND col2 < $3)
)

```
which allows for DESC order keys:
```
AND (
  (col1 < $4) OR
  (col1 = $4 AND col2 <= $5)
)
AND (
  (col1 > $2) OR
  (col1 = $2 AND col2 > $3)
)
```
or mixed ASC/DESC order keys:
```
AND (
  (col1 > $4) OR
  (col1 = $4 AND col2 <= $5)
)
AND (
  (col1 < $2) OR
  (col1 = $2 AND col2 > $3)
)
```

The `ORDER BY` clause also replaces implicit ascending ordering:
```
ORDER BY col1, col2
```
with explicit ordering:
```
ORDER BY col1 ASC, col2 DESC
```

Release note (sql change): TTL supports DESC order primay key columns.

Depends on
~#102030
~#102127

102209: sqlstats: improve `GetPercentileValues` performance r=maryliag a=maryliag

Previously, the function to Query the percentile values
was also doing a flush of the stream. This was required
because during Insights detector, everytime some data was
added a query was done after, so a flush had to be done to
guarantee precision.
When making the usage of the same Query function for the
`GetPercentileValues` it was still doing a flush, but that
was not necessary, since there was no new data, and if there
was the detector was going to take care of the flush. It could
also be causing contention on the stream.

Since there is no need for the flush when retrieving the info
for sql stats, this commits updates the Query function
to have an optional flush, that can be skipped on that case.
Probably other improvements can be made on the existing path,
but this PR focus on the performance degradations caused by
the function `GetPercentileValues` and no behaviour change
is made on other existing flows.

Fixes #102208

Before
<img width="1218" alt="current-total" src="https://user-images.githubusercontent.com/1017486/234451031-d37a85c0-b322-497b-8d0c-a8851a8cffe8.png">


After
<img width="870" alt="op-flush-total" src="https://user-images.githubusercontent.com/1017486/234451041-5b9104a9-8663-4322-a2a2-f7a979bfa76b.png">

---

Before
<img width="875" alt="current-pink" src="https://user-images.githubusercontent.com/1017486/234451225-0b552281-d162-4e64-9d37-80bec61e4810.png">


After
<img width="871" alt="op-flush-pink" src="https://user-images.githubusercontent.com/1017486/234451243-6838cc19-aeaf-4f26-9a2f-7c391f6aba79.png">

---

Before
<img width="868" alt="current" src="https://user-images.githubusercontent.com/1017486/234451261-a12736ec-baec-44ef-9004-884c136b484b.png">

After
<img width="808" alt="op-flush" src="https://user-images.githubusercontent.com/1017486/234451278-85178109-65f5-4ad0-82a2-2ecb3e786103.png">

---

Before
<img width="1613" alt="current-query" src="https://user-images.githubusercontent.com/1017486/234451301-e59241d8-a089-440b-9a23-3d36ba5413d7.png">

After
<img width="924" alt="op-flush-query" src="https://user-images.githubusercontent.com/1017486/234451327-19d95775-0443-4f81-a4c7-d3a6c3253245.png">

Release note: None

102297: flowinfra: remove stale TODO r=yuzefovich a=yuzefovich

I don't think the code movement proposed in the TODO makes sense anymore.

Epic: None

Release note: None

Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multitenant: new 23.2 versions break tenant_upgrade_test
3 participants