Skip to content

Conversation

@sumeerbhola
Copy link
Collaborator

Epic: CRDB-55052

Release note: None

@sumeerbhola sumeerbhola requested review from tbg and wenyihu6 November 13, 2025 21:27
@sumeerbhola sumeerbhola requested review from a team as code owners November 13, 2025 21:27
@blathers-crl
Copy link

blathers-crl bot commented Nov 13, 2025

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

:lgtm:

I don't love that we're making these changes without accompanying unit tests, but I understand the time commitment that backfilling this would take. Something that's on my mind and that should get better as I continue my crusade of breaking things up into smaller functions.

I will send a follow-up (that I stacked on top of this PR) to distinguish between signed and unsigned load values and to use package boundaries to make sure values that could become negative are never left unhandled.

bors r+

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


-- commits line 2 at r1:
What keeps us from preventing nodes to become negative in the first place? That strikes me as more structurally sound. In this PR, the TODOs are gone and there are some places that handle negative load, but it's now a cognitive burden carried around everywhere, not to mention the realistic potential for this to regress.
If we clamp the load at zero, we lose the fact that load adding is just pure arithmetic. It won't be true, for example, that a + b - b equals a. I assume this is going to be annoying in other places. I will assume that you had considered that option too, but discarded it on those grounds.

One way we can do better with current approach: we add a type SignedLoadVector which is allowed to be negative and use that for adjusted. Then we have a method Nonnegative() or Clamp() or whatever that returns a LoadVector (=unsigned load vector) - and we make sure that is what we use whenever we

@craig
Copy link
Contributor

craig bot commented Nov 14, 2025

@craig craig bot merged commit 60165a6 into cockroachdb:master Nov 14, 2025
23 checks passed
Copy link
Collaborator Author

@sumeerbhola sumeerbhola 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! 1 of 0 LGTMs obtained


-- commits line 2 at r1:

If we clamp the load at zero, we lose the fact that load adding is just pure arithmetic. It won't be true, for example, that a + b - b equals a. I assume this is going to be annoying in other places. I will assume that you had considered that option too, but discarded it on those grounds.

Exactly. We need reversible computation.

One way we can do better with current approach: we add a type SignedLoadVector which is allowed to be negative and use that for adjusted.

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants