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

receiver: avoid race of hashring #1371

Merged
merged 1 commit into from
Aug 5, 2019
Merged

Conversation

YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Aug 5, 2019

Signed-off-by: YaoZengzeng yaozengzeng@zju.edu.cn

@YaoZengzeng
Copy link
Contributor Author

@squat PTAL

@@ -140,6 +143,9 @@ func (h *Handler) StorageReady() {
// Hashring sets the hashring for the handler and marks the hashring as ready.
// If the hashring is nil, then the hashring is marked as not ready.
func (h *Handler) Hashring(hashring Hashring) {
h.mtx.Lock()
defer h.mtx.Unlock()

if hashring == nil {
atomic.StoreUint32(&h.hashringReady, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Now that we have introduced a new syncronization mechanism, we can get rid of the hashringReady field, which we were accessing atomically to determine hashring readiness. Instead, isReady can do a RLock, e.g.

h.mtx.RLock()
hr := h.Hashring != nil
h.mtx.RUnlock()
return sr > 0 && hr

// It is possible that hashring is ready in testReady() but become unready now,
// so we need to lock here.
if h.hashring == nil {
h.mtx.RLock()
Copy link
Member

Choose a reason for hiding this comment

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

This should be RUnlock

@@ -400,14 +417,25 @@ func (h *Handler) replicate(ctx context.Context, tenant string, wreq *prompb.Wri
wreqs := make(map[string]*prompb.WriteRequest)
replicas := make(map[string]replica)
var i uint64

h.mtx.RLock()
Copy link
Member

Choose a reason for hiding this comment

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

Just for clarity, plz move this locking below the comment.

@@ -275,6 +281,15 @@ func (h *Handler) receive(w http.ResponseWriter, r *http.Request) {
func (h *Handler) forward(ctx context.Context, tenant string, r replica, wreq *prompb.WriteRequest) error {
wreqs := make(map[string]*prompb.WriteRequest)
replicas := make(map[string]replica)

h.mtx.RLock()
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this below the comment.

Signed-off-by: YaoZengzeng <yaozengzeng@zju.edu.cn>
@YaoZengzeng
Copy link
Contributor Author

@squat Updated.

@squat
Copy link
Member

squat commented Aug 5, 2019

cc @brancz for approval and merge

@brancz
Copy link
Member

brancz commented Aug 5, 2019

Thanks a lot for finding and fixing this @YaoZengzeng!

@brancz brancz merged commit 921aa0b into thanos-io:master Aug 5, 2019
paulfantom added a commit to paulfantom/thanos that referenced this pull request Aug 7, 2019
* master:
  iter.go: error message typo correction (thanos-io#1376)
  Fix usage of $GOPATH in Makefile (thanos-io#1379)
  Moved Prometheus 2.11.1 and TSDB to 0.9.1 (thanos-io#1380)
  Store latest config hash and timestamp as metrics (thanos-io#1378)
  pkg/receive/handler.go: log errors (thanos-io#1372)
  receive: Hash-ring metrics (thanos-io#1363)
  receiver: avoid race of hashring (thanos-io#1371)
  feat compact: added readiness Prober (thanos-io#1297)
  Add changelog entry for S3 option (thanos-io#1361)
  Multipart features (thanos-io#1358)
  Added katacoda.yaml (thanos-io#1359)
  Remove deprecated option from example (thanos-io#1351)
  Move suggestion about admin API to appropriate place (thanos-io#1355)
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