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: do not show current time as a fall back for empty timestamps on Jobs pages #110366

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

koorosh
Copy link
Collaborator

@koorosh koorosh commented Sep 11, 2023

Previously, timestamps for running jobs showed
current time even if job is not finished because
TimestampToMoment function used current time as
a default value for Nulls.
With this change it is explicitly set to Null to
make sure that Job's time is not misinterpreted.

Resolves: #109204

Release note (ui change): Correctly display timestamps
for creation, last modified, and completed time on Jobs table.

Screenshot 2023-09-11 at 22 10 13

@blathers-crl
Copy link

blathers-crl bot commented Sep 11, 2023

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

@blathers-crl blathers-crl bot added the O-community Originated from the community label Sep 11, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

why on your image the alter tables don't have a completed time? Shouldn't they have it? The issue was focusing on jobs being executed, so I would also like to see examples of that

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

Copy link
Collaborator Author

@koorosh koorosh left a comment

Choose a reason for hiding this comment

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

I made backend to return job with empty finished_at field just to simplify testing as it is pure front-end issue so that's why completed job doesn't have "Completed time".
In addition I'd like to highlight that the problem is not specifically related for running jobs. The problem in general is that frontend changed timestamp's values from 'NULL' to current time.
With current change Jobs table display the same data as SHOW JOBS query.

Appreciate any help on how to have a long running job for testing under real conditions 🙏🏽

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @j82w, @THardy98, @xinhaoz, and @zachlite)

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

@yuzefovich would you mind giving some repro steps from the issue you created? How to get a long running job so Andrii can test his changes?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @j82w, @THardy98, @xinhaoz, and @zachlite)

@yuzefovich
Copy link
Member

I don't have reproduction steps - I noticed this while looking at DB Console of one of existing CC clusters (that I was investigating for a different reason). Some context can be found in https://cockroachlabs.slack.com/archives/C04N0AS14CT/p1692666351682189. Perhaps @fqazi can help with a repro.

Copy link
Contributor

@maryliag maryliag 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 1 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @gtr, @j82w, @THardy98, @xinhaoz, and @zachlite)

Previously, timestamps for running jobs showed
current time even if job is not finished because
`TimestampToMoment` function used current time as
a default value for Nulls.
With this change it is explicitly set to Null to
make sure that Job's time is not misinterpreted.

Release note (ui change): Correctly display timestamps
for creation, last modified, and completed time on Jobs table.
@blathers-crl
Copy link

blathers-crl bot commented Oct 5, 2023

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

@maryliag maryliag added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Oct 5, 2023
@koorosh
Copy link
Collaborator Author

koorosh commented Oct 5, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 5, 2023

Build succeeded:

@craig craig bot merged commit bc191fd into cockroachdb:master Oct 5, 2023
8 checks passed
@blathers-crl
Copy link

blathers-crl bot commented Oct 5, 2023

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 3e37378 to blathers/backport-release-23.1-110366: 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.

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 O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: currently running job uses now() time as Completed Time
4 participants