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

allocatorimpl: vlog on all excl. repl due to catchup conditions #132603

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Oct 14, 2024

Previously, we would only VEventf(ctx, 5, ...) when a replica was
being excluded due to having a send queue, or not being in
StateReplicate.

Now also:

  • V(4) log when a replica is included due to missing stats.
  • V(6) log when a replica is included due to passing stats.

Also, stop shadowing the range stat declaration with the replica
stat declaration within the loop.

Part of: #123509
Release note: None

@kvoli kvoli self-assigned this Oct 14, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

kvoli added 2 commits October 14, 2024 15:51
Add `String()` and `SafeFormat()` methods for `RangeSendStreamStats` and
`ReplicaSendStreamStats`.

Part of: cockroachdb#123509
Release note: None
Previously, we would only `VEventf(ctx, 5, ...)` when a replica was
being excluded due to having a send queue, or not being in
`StateReplicate`.

Now also:
- `V(4)` log when a replica is included due to missing stats.
- `V(6)` log when a replica is included due to passing stats.

Part of: cockroachdb#123509
Release note: None
@kvoli kvoli force-pushed the 241014.rac2-send-stream-stats-logging branch from e98f773 to 2bacf24 Compare October 14, 2024 19:51
@kvoli kvoli marked this pull request as ready for review October 14, 2024 21:28
@kvoli kvoli requested review from a team as code owners October 14, 2024 21:28
@kvoli kvoli requested a review from sumeerbhola October 14, 2024 21:28
Copy link
Collaborator

@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.

:lgtm:

Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)


pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 224 at r1 (raw file):

			w.Printf(", ")
		}
		w.Printf("r%v=(%v)", s.internal[i].ReplicaID, s.internal[i])

Is the convention that the r prefix is used only for the range-id?


pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 1436 at r1 (raw file):

		panic(errors.AssertionFailedf("statsToSet is non-empty %v", statsToSet.internal))
	}
	statsToSet.Clear()

was this unnecessary because of:

	// INVARIANT: len(stats.internal) = 0.

Copy link
Collaborator Author

@kvoli kvoli left a comment

Choose a reason for hiding this comment

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

TYFTR!

bors r=sumeerbhola

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)


pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 224 at r1 (raw file):

Previously, sumeerbhola wrote…

Is the convention that the r prefix is used only for the range-id?

I've seen it also used for replicas in some contexts, the allocator being one of them.


pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 1436 at r1 (raw file):

Previously, sumeerbhola wrote…

was this unnecessary because of:

	// INVARIANT: len(stats.internal) = 0.

Yeah -- I forgot to add this originally.

@craig craig bot merged commit 1256394 into cockroachdb:master Oct 24, 2024
23 checks passed
@kvoli
Copy link
Collaborator Author

kvoli commented Oct 25, 2024

blathers backport 24.3

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.

3 participants