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

Implement quorum reads and merging for /v2/silences and /v2/silence/id #4141

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

stevesg
Copy link
Contributor

@stevesg stevesg commented Apr 28, 2021

What this PR does:

Reading silence listings and individual silences will now read from a
quorum of replicas and return a merged response. In both cases, in the
presence of duplicate silences with the same "id", the silence with the
most recent "updatedAt" timestamp is return.

Note that this does not yet bring full consistency to silences, as we
are not able to perform quorum writing of silences without upstream
changes to Alertmanager.

However, it will still be an improvement:

  • Reads will be consistent more often, being able to take state from
    multiple replicas instead of just one.
  • Requests will be resilient to single replica failure, as the request
    is essentially attempted on all three replicas.

A possible extension to this change would be to block for all replicas
to reply, but still allow a single failure. This would mean that reads
are consistent for the case when all replicas are contactable.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

…id}.

Reading silence listings and individual silences will now read from a
quorum of replicas and return a merged response. In both cases, in the
presence of duplicate silences with the same "id", the silence with the
most recent "updatedAt" timestamp is return.

Note that this does not yet bring full consistency to silences, as we
are not able to perform quorum writing of silences without upstream
changes to Alertmanager.

However, it will still be an improvement:
- Reads will be consistent more often, being able to take state from
  multiple replicas instead of just one.
- Requests will be resilient to single replica failure, as the request
  is essentially attempted on all three replicas.

An possible extension to this change would be to block for all replicas
to reply, but still allow a single failure. This would mean that reads
are consistent for the case when all replicas are contactable.

Signed-off-by: Steve Simpson <[email protected]>
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Nice job!

(nit: Having one type per file seems like Javaism, and not something we normally do in Go. But it explains why you created whole new package for it :). Perhaps we can merge all v2 stuff into single v2.go file?)

@stevesg
Copy link
Contributor Author

stevesg commented Apr 29, 2021

(nit: Having one type per file seems like Javaism, and not something we normally do in Go. But it explains why you created whole new package for it :). Perhaps we can merge all v2 stuff into single v2.go file?)

Haha, fair comment. When I started the mergers were a lot more complicated, so I think it made sense.

(However, and old colleague of mine said the opposite - "In Go we have no problem breaking code up into lots of files"!)

Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Fantastic job! Keep doing such great work! 👏 👏 👏

@pracucci pracucci merged commit 653affe into cortexproject:master Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants