Refactor replicate execution method to not iterate the origin bucket by yeya24 · Pull Request #2906 · thanos-io/thanos

yeya24 · 2020-07-17T21:50:33Z

Signed-off-by: Ben Ye yb532204897@gmail.com

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

Changes

In the previous implementation, we are iterating the origin bucket, and each time, we use metaFetcher to fetch all meta files. But this seems not necessary, we can just fetch meta files once at the beginning.

Verification

GiedriusS

Two comments but overall LGTM!

pkg/replicate/scheme.go

GiedriusS · 2020-07-17T22:45:57Z

pkg/replicate/scheme.go

+	metas, partials, err := rs.fetcher.Fetch(ctx)
+	if err != nil && metas == nil {
+		return err
+	}


Perhaps we should log the error if metas != nil && err != nil?

It is good to log the error here, but kind of duplicate.

I just check the code, only here returns metas and error at the same time. In this case, partials != nil, so we will log the partial meta errors later.

hm the if err != nil && metas == nil { feels like code smell to me though. It just looks wrong from outside which can lead to bugs in future:

Inside implementation can change, and metas can be produced without partial for some reason - then we will miss an error.

Even if we say it makes sense, maybe another person will come here and refactor it anyway as it looks like error can be missed.

On other hand, shouldn't replicate FAIL on any error spotted? Even if we failed to read one block? Sometimes we heavily rely on replication status so this has to be heavily tested and controlled, ideally with failed metric being somewhere (:

yeya24 · 2020-07-19T19:32:26Z

CI failure is unrelated. Please take a look at this pr again

GiedriusS

AFAICT the CI failure seems related to the changes

yeya24 · 2020-07-29T20:15:07Z

AFAICT the CI failure seems related to the changes

Thanks, the previous problem was caused by the make examples-in-container target, and that should be fixed it in the latest commit 😄 .

pkg/replicate/scheme.go

OGKevin

LGTM

yeya24 · 2020-09-02T00:47:06Z

Can I get a review for this? I think it is close.

kakkoyun

lgtm

bwplotka

Nice, some comments though, before merging (:

pkg/replicate/scheme.go

bwplotka · 2020-09-11T16:45:40Z

pkg/replicate/scheme.go

+	metas, partials, err := rs.fetcher.Fetch(ctx)
+	if err != nil && metas == nil {
+		return err
+	}


hm the if err != nil && metas == nil { feels like code smell to me though. It just looks wrong from outside which can lead to bugs in future:

Inside implementation can change, and metas can be produced without partial for some reason - then we will miss an error.

Even if we say it makes sense, maybe another person will come here and refactor it anyway as it looks like error can be missed.

On other hand, shouldn't replicate FAIL on any error spotted? Even if we failed to read one block? Sometimes we heavily rely on replication status so this has to be heavily tested and controlled, ideally with failed metric being somewhere (:

bwplotka · 2020-09-11T16:46:19Z

pkg/replicate/scheme.go

-			return nil
-		}
+	for id, partialError := range partials {
+		level.Info(rs.logger).Log("msg", "failed to fetch block meta. Skipping.", "block_uuid", id.String(), "err", partialError)


Is this failure? This is actually valid state of blocks being potentially uploaded, no?

Something like level.Info(rs.logger).Log("msg", "block meta not uploaded yet. Skipping.", "block_uuid", id.String()) r

PartialError has 2 cases, one is meta.json not found, another one is meta.json corrupted. What about the corrupted case, is it an error or not?

For different obj store implementation, is it possible that one uploaded file is corrupted? If it is this case, how can we differentiate the case that one file is totally corrupted or still being uploaded?

corrupted case, is it an error or not?

For current flow it's not error. We consider corrupted meta same as no meta.json as the reason is usually same - partial upload (:

Thanks! Updated.

kakkoyun · 2020-10-19T13:13:03Z

@yeya24 This looks fine to me. You need to rebase I believe.
Ping @bwplotka are you satisfied with the state of the PR now?

bwplotka

Responded to comment, that still needs to be addressed, otherwise LGTM!

Thanks!

bwplotka · 2020-10-30T19:44:32Z

pkg/replicate/scheme.go

-			return nil
-		}
+	for id, partialError := range partials {
+		level.Info(rs.logger).Log("msg", "failed to fetch block meta. Skipping.", "block_uuid", id.String(), "err", partialError)


corrupted case, is it an error or not?

For current flow it's not error. We consider corrupted meta same as no meta.json as the reason is usually same - partial upload (:

Signed-off-by: Ben Ye <yb532204897@gmail.com>

bwplotka

Thanks!

Changelog entry could be better, but we can fix it later.

bwplotka · 2020-10-30T20:54:14Z

CHANGELOG.md


+### Changed
+
+- [#2906](https://github.com/thanos-io/thanos/pull/2906) Tools: Refactor Bucket replicate execution. Removed all `thanos_replicate_origin_.*` metrics.


Technically not need to mention refactor, just metric changes (:

OGKevin · 2020-10-30T20:58:47Z

Whoop 🥳 🎊 I'll work on #2979.

OGKevin · 2020-10-30T20:59:06Z

Nice work @yeya24

bwplotka · 2020-10-30T21:02:06Z

bwplotka · 2020-10-30T21:02:15Z

💪

…hanos-io#2906) * Refactor replicate execution Signed-off-by: Ben Ye <yb532204897@gmail.com> * return error when fetcher gets an error Signed-off-by: Ben Ye <yb532204897@gmail.com> * address feedback Signed-off-by: Ben Ye <yb532204897@gmail.com> Signed-off-by: Oghenebrume50 <raphlbrume@gmail.com>

GiedriusS reviewed Jul 17, 2020

View reviewed changes

yeya24 force-pushed the rework-replicate-exec branch from eb9fe91 to 2d6d058 Compare July 29, 2020 15:59

GiedriusS reviewed Jul 29, 2020

View reviewed changes

yeya24 force-pushed the rework-replicate-exec branch from af3a2a8 to 7eafd1c Compare July 29, 2020 20:08

OGKevin reviewed Aug 7, 2020

View reviewed changes

pkg/replicate/scheme.go Outdated Show resolved Hide resolved

pkg/replicate/scheme.go Show resolved Hide resolved

yeya24 force-pushed the rework-replicate-exec branch from 7eafd1c to e5f0737 Compare August 8, 2020 18:59

OGKevin approved these changes Aug 10, 2020

View reviewed changes

OGKevin mentioned this pull request Aug 10, 2020

REPLICATOR: Add TimePartitionMetaFilter to thanos replicate. #2979

Merged

2 tasks

yeya24 force-pushed the rework-replicate-exec branch from e5f0737 to 4b15ae5 Compare August 10, 2020 12:11

yeya24 mentioned this pull request Aug 19, 2020

Feature Request: Replicate can copy individual blocks #2713

Closed

yeya24 force-pushed the rework-replicate-exec branch from 4b15ae5 to f89065a Compare September 9, 2020 00:51

yeya24 requested review from GiedriusS and kakkoyun September 9, 2020 00:52

yeya24 force-pushed the rework-replicate-exec branch from f89065a to 8d2e154 Compare September 9, 2020 01:08

kakkoyun approved these changes Sep 9, 2020

View reviewed changes

yeya24 force-pushed the rework-replicate-exec branch from 8d2e154 to a75bef5 Compare September 11, 2020 13:03

yeya24 requested review from bwplotka and daixiang0 September 11, 2020 13:09

bwplotka reviewed Sep 11, 2020

View reviewed changes

yeya24 force-pushed the rework-replicate-exec branch from ddc233c to c8140be Compare October 19, 2020 13:29

bwplotka approved these changes Oct 30, 2020

View reviewed changes

yeya24 added 3 commits October 30, 2020 15:55

Refactor replicate execution

6523767

Signed-off-by: Ben Ye <yb532204897@gmail.com>

return error when fetcher gets an error

03cb696

Signed-off-by: Ben Ye <yb532204897@gmail.com>

address feedback

f1079be

Signed-off-by: Ben Ye <yb532204897@gmail.com>

yeya24 force-pushed the rework-replicate-exec branch from c8140be to f1079be Compare October 30, 2020 19:57

bwplotka approved these changes Oct 30, 2020

View reviewed changes

bwplotka merged commit 5abda52 into thanos-io:master Oct 30, 2020

yeya24 deleted the rework-replicate-exec branch October 30, 2020 22:20


		### Changed

		- [#2906](https://github.com/thanos-io/thanos/pull/2906) Tools: Refactor Bucket replicate execution. Removed all `thanos_replicate_origin_.*` metrics.

Conversation

yeya24 commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Verification

Uh oh!

GiedriusS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeya24 commented Jul 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GiedriusS left a comment

Choose a reason for hiding this comment

Uh oh!

yeya24 commented Jul 29, 2020

Uh oh!

Uh oh!

Uh oh!

OGKevin left a comment

Choose a reason for hiding this comment

Uh oh!

yeya24 commented Sep 2, 2020

Uh oh!

kakkoyun left a comment

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kakkoyun commented Oct 19, 2020

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OGKevin commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OGKevin commented Oct 30, 2020

Uh oh!

bwplotka commented Oct 30, 2020

Uh oh!

bwplotka commented Oct 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yeya24 commented Jul 17, 2020 •

edited

Loading

yeya24 commented Jul 19, 2020 •

edited

Loading

OGKevin commented Oct 30, 2020 •

edited

Loading