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

jobs: change job_info.info_key to string #99878

Merged
merged 1 commit into from
Mar 30, 2023
Merged

Conversation

dt
Copy link
Member

@dt dt commented Mar 28, 2023

Release note: none.
Epic: none.

Hopefully we get this one in now before it is released and harder to change later. I think if we go with bytes, we'll spend the next several years typing convert_to over and over, or forgetting to and then typing it, when debugging.

@dt dt requested review from adityamaru and a team March 28, 2023 22:41
@dt dt requested a review from a team as a code owner March 28, 2023 22:41
@dt dt requested a review from a team March 28, 2023 22:41
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt dt force-pushed the job_info_type branch 3 times, most recently from 64e54ad to 7ec8080 Compare March 29, 2023 01:22
@dt
Copy link
Member Author

dt commented Mar 29, 2023

removed the upgrade migration from this; if we get it into 23.1 before the beta, arguably we don't need it since that'll be the first release from which we need to be upgradable, and in that case we don't need the migration. If we did need to be upgradable, this PR would need to be much more complex and wrap every info table query in a conditional to cast or not cast to BYTES. Since it isn't doing that, there's no point in having the migration.

@dt
Copy link
Member Author

dt commented Mar 29, 2023

TFTRs!

bors r+

(red CI is green on everything except maybe_stress which is apparently just known to be busted)

@dt
Copy link
Member Author

dt commented Mar 29, 2023

blathers backport 23.1

@blathers-crl
Copy link

blathers-crl bot commented Mar 29, 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 7ec8080 to blathers/backport-release-23.1-99878: 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 failed. See errors above.


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

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Build failed (retrying...):

@yuzefovich
Copy link
Member

This likely needs a rebase.

bors r-

pkg/jobs/jobsprofiler/profiler.go:47:34: cannot use []byte(fmt.Sprintf(infoKey, timeutil.Now().UnixNano())) (value of type []byte) as type string in argument to infoStorage.Write

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Canceled.

@dt
Copy link
Member Author

dt commented Mar 29, 2023

thanks @yuzefovich!

This likely needs a rebase.
done.

bors r+

@dt
Copy link
Member Author

dt commented Mar 29, 2023

don't think bors noticed the comment since it didn't queue it.

bors r+

@knz
Copy link
Contributor

knz commented Mar 29, 2023

bors r-

please rebase. there's a merge conflict now.

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Canceled.

Release note: none.
Epic: none.

Hopefully we get this one in now before it is released and harder to change later.
I think if we go with bytes, we'll spend the next several years typing convert_to
over and over, or forgetting to and then typing it, when debugging.
@dt
Copy link
Member Author

dt commented Mar 29, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 30, 2023

Build succeeded:

@craig craig bot merged commit 99b655d into cockroachdb:master Mar 30, 2023
@dt dt deleted the job_info_type branch March 30, 2023 12:19
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.

6 participants