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

Add error log to receiver and clarify bad replica error message #4394

Merged
merged 5 commits into from
Jul 2, 2021

Conversation

bill3tt
Copy link
Contributor

@bill3tt bill3tt commented Jul 1, 2021

Signed-off-by: Ian Billett ibillett@redhat.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Fixes #4305

Verification

Running the integration tests from #4362 I see the following log:

11:25:28 receive-i3: level=error name=receive-i3 ts=2021-07-01T10:25:28.330239422Z caller=handler.go:261 component=receive component=receive-handler err="request replica exceeds receiver replication factor" msg="write request rejected" request_replica=2 replication_factor=1

Signed-off-by: Ian Billett <ibillett@redhat.com>
Signed-off-by: Ian Billett <ibillett@redhat.com>
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

lgtm (:

@squat squat enabled auto-merge (squash) July 1, 2021 10:31
Signed-off-by: Ian Billett <ibillett@redhat.com>
Signed-off-by: Ian Billett <ibillett@redhat.com>
@@ -258,6 +258,8 @@ type replica struct {
func (h *Handler) handleRequest(ctx context.Context, rep uint64, tenant string, wreq *prompb.WriteRequest) error {
// The replica value in the header is one-indexed, thus we need >.
if rep > h.options.ReplicationFactor {
level.Error(h.logger).Log("err", errBadReplica, "msg", "write request rejected",
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this in the place where error is handled? Handling error twice (by returning and logging) is a bad smell 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could 👍 I chose not to as we would then need to replicate this in two places in a switch statement (here & here).

I felt this was slightly tidier, but happy to be convinced otherwise 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Hm... there is a risk we will handle things twice, but not a biggie! (:

Signed-off-by: Ian Billett <ibillett@redhat.com>
@@ -258,6 +258,8 @@ type replica struct {
func (h *Handler) handleRequest(ctx context.Context, rep uint64, tenant string, wreq *prompb.WriteRequest) error {
// The replica value in the header is one-indexed, thus we need >.
if rep > h.options.ReplicationFactor {
level.Error(h.logger).Log("err", errBadReplica, "msg", "write request rejected",
Copy link
Member

Choose a reason for hiding this comment

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

Hm... there is a risk we will handle things twice, but not a biggie! (:

@squat squat merged commit 55d2775 into thanos-io:main Jul 2, 2021
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.

receive: Replica count exceeds replication factor although it does not?
3 participants