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

ui: improve db page nodes/regions query #118904

Merged

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Feb 7, 2024

This commit rewrites the query to fetch node and region information for a db
in the databases page in db-console. The runtime of the query is improved
by removing the expressions used to extract the region information in the
replica_locality column of the ranges table since this field was not used
in the client.

The function createMockDatabaseRangesForTable in fakeApi.ts is also
deleted as it has no callers.

Part of: #114690

Release note: None


https://www.loom.com/share/cec82ea6c26849e28cfea31d66fdc719

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz force-pushed the improve-db-page-replica-localities branch 2 times, most recently from 56b98e1 to f2ac4b5 Compare February 8, 2024 18:23
@xinhaoz xinhaoz marked this pull request as ready for review February 8, 2024 18:23
@xinhaoz xinhaoz requested review from a team and abarganier and removed request for a team February 8, 2024 18:23
@xinhaoz xinhaoz force-pushed the improve-db-page-replica-localities branch 2 times, most recently from 39fd668 to ba6260f Compare February 8, 2024 23:36
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 82 at r2 (raw file):

  const dbStats = dbDetails?.data?.results.stats;
  // TODO #118957 (xinhaoz) Use store id to regions mapping.
  const nodes = dbStats?.replicaData.storeIDs || [];

can this variable be stores? or is that work that will be in a follow up PR? The way this reads suggests that if store and node IDs don't match the code won't work.

@xinhaoz xinhaoz force-pushed the improve-db-page-replica-localities branch from ba6260f to 10b8c2a Compare February 8, 2024 23:54
@xinhaoz
Copy link
Member Author

xinhaoz commented Feb 9, 2024

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 82 at r2 (raw file):

Previously, dhartunian (David Hartunian) wrote…

can this variable be stores? or is that work that will be in a follow up PR? The way this reads suggests that if store and node IDs don't match the code won't work.

Was planning on renaming it when resolving #118957, but I can change it now, too (have to repush anyways to resolve a test).

@xinhaoz xinhaoz force-pushed the improve-db-page-replica-localities branch from 10b8c2a to 051c7b7 Compare February 9, 2024 23:43
@xinhaoz xinhaoz requested a review from dhartunian February 12, 2024 16:07
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 4 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts line 370 at r3 (raw file):

          `
            SELECT array_agg(DISTINCT unnested) AS store_ids
            FROM [SHOW RANGES FROM DATABASE %1], unnest(replicas) AS unnested

nit: can you call this something more specific like unnested_store_ids? Reading DISTINCT unnested in the SELECT is confusing.


pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts line 383 at r3 (raw file):

        resp.stats.replicaData.error = txn_result.error;
      }
      resp.stats.replicaData.storeIDs = txn_result?.rows[0]?.store_ids ?? [];

should we be returning from the error clause? and seeing .storeIDs to empty there? Can we avoid some of the ? in that case and assume we have some data?

This commit rewrites the query to fetch node and region information for a db
in the databases page in db-console. The runtime of the query is improved
by removing the expressions used to extract the region information in the
`replica_locality` column of the ranges table since this field was not used
in the client.

The function `createMockDatabaseRangesForTable` in `fakeApi.ts` is also
deleted as it has no callers.

Part of: cockroachdb#114690

Release note: None
@xinhaoz xinhaoz force-pushed the improve-db-page-replica-localities branch from 051c7b7 to bcf2d35 Compare February 26, 2024 17:20
Copy link
Member Author

@xinhaoz xinhaoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @dhartunian)


pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts line 370 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: can you call this something more specific like unnested_store_ids? Reading DISTINCT unnested in the SELECT is confusing.

Done.


pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts line 383 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

should we be returning from the error clause? and seeing .storeIDs to empty there? Can we avoid some of the ? in that case and assume we have some data?

I kept the previous behaviour of not returning on error since the error could be something where we can still show partial results (e.g. max size exceeded).

@xinhaoz xinhaoz requested a review from dhartunian February 26, 2024 18:13
@xinhaoz xinhaoz added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Feb 26, 2024
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @abarganier)

@xinhaoz
Copy link
Member Author

xinhaoz commented Feb 29, 2024

tftr!
bors r+

@craig
Copy link
Contributor

craig bot commented Feb 29, 2024

Build succeeded:

@craig craig bot merged commit 4d65905 into cockroachdb:master Feb 29, 2024
15 checks passed
Copy link

blathers-crl bot commented Feb 29, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from bcf2d35 to blathers/backport-release-23.1-118904: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


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

@xinhaoz xinhaoz deleted the improve-db-page-replica-localities branch April 1, 2024 17:02
xinhaoz added a commit to xinhaoz/cockroach that referenced this pull request Jul 1, 2024
These queries were updated in cockroachdb#118904 to make them more performant.
In that change we mistakenly removed the null check before reading
the `rows` field, causing pages using this request to crash if this
query fails.

Epic: none
Fixes: cockroachdb#126348

Release note (bug fix): Database overview and db details
pages should not crash if the range information is not
available.
xinhaoz added a commit to xinhaoz/cockroach that referenced this pull request Jul 8, 2024
These queries were updated in cockroachdb#118904 to make them more performant.
In that change we mistakenly removed the null check before reading
the `rows` field, causing pages using this request to crash if this
query fails.

Epic: none
Fixes: cockroachdb#126348

Release note (bug fix): Database overview and db details
pages should not crash if the range information is not
available.
craig bot pushed a commit that referenced this pull request Jul 8, 2024
126336: sql: override StmtTimeout to 0 for background jobs r=annrpom a=annrpom

For background jobs and schema changes, our Internal
Executor inherits from cluster settings. If a statement
timeout is set cluster-wide, then background jobs will
respect this -- which does not make much sense; thus,
this patch overrides the StmtTimeout to 0s for background
jobs.

Fixes: #126261
Release note (bug fix): Background jobs no longer respect
a statement timeout.

126419: ui: fix query fetching store ids for db/tables r=xinhaoz a=xinhaoz

These queries were updated in #118904 to make them more performant. In that change we mistakenly removed the null check before reading the `rows` field, causing pages using this request to crash if this query fails.

Epic: none
Fixes: #126348

Release note (bug fix): Database overview and db details pages should not crash if the range information is not available.

126575: kv: add support for Lease.MinExpiration r=nvanbenschoten a=nvanbenschoten

Closes #125235.

This commit begins using the new `Lease.MinExpiration` field, which was introduced in 7781cfa. `Lease.MinExpiration` is assigned to epoch-based leases (and soon, leader leases) during upgrades from expiration-based lease. It is a simple, direct way to ensure that the expiration of the lease does not regress during such promotions.

We previously solved this issue in 6dd54b4 by manually heartbeating the node liveness record when we detected an expiration regression. This was backwards compatible, so it was a better backport target, but it was also complex, as demonstrated by the need for b2ac52a. In the future, we will be able to remove the manual heartbeat logic, as it is no longer necessary once MinTimestamp is supported by all clusters.

Epic: None

Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
asg0451 pushed a commit to asg0451/cockroach that referenced this pull request Jul 10, 2024
These queries were updated in cockroachdb#118904 to make them more performant.
In that change we mistakenly removed the null check before reading
the `rows` field, causing pages using this request to crash if this
query fails.

Epic: none
Fixes: cockroachdb#126348

Release note (bug fix): Database overview and db details
pages should not crash if the range information is not
available.
xinhaoz added a commit to xinhaoz/cockroach that referenced this pull request Jul 11, 2024
These queries were updated in cockroachdb#118904 to make them more performant.
In that change we mistakenly removed the null check before reading
the `rows` field, causing pages using this request to crash if this
query fails.

Epic: none
Fixes: cockroachdb#126348

Release note (bug fix): Database overview and db details
pages should not crash if the range information is not
available.
xinhaoz added a commit to xinhaoz/cockroach that referenced this pull request Jul 11, 2024
These queries were updated in cockroachdb#118904 to make them more performant.
In that change we mistakenly removed the null check before reading
the `rows` field, causing pages using this request to crash if this
query fails.

Epic: none
Fixes: cockroachdb#126348

Release note (bug fix): Database overview and db details
pages should not crash if the range information is not
available.
xinhaoz added a commit to xinhaoz/cockroach that referenced this pull request Jul 11, 2024
These queries were updated in cockroachdb#118904 to make them more performant.
In that change we mistakenly removed the null check before reading
the `rows` field, causing pages using this request to crash if this
query fails.

Epic: none
Fixes: cockroachdb#126348

Release note (bug fix): Database overview and db details
pages should not crash if the range information is not
available.
xinhaoz added a commit to xinhaoz/cockroach that referenced this pull request Jul 16, 2024
These queries were updated in cockroachdb#118904 to make them more performant.
In that change we mistakenly removed the null check before reading
the `rows` field, causing pages using this request to crash if this
query fails.

Epic: none
Fixes: cockroachdb#126348

Release note (bug fix): Database overview and db details
pages should not crash if the range information is not
available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants