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

REPLICATOR: Add TimePartitionMetaFilter to thanos replicate. #2979

Merged
merged 10 commits into from
Nov 3, 2020

Conversation

OGKevin
Copy link
Contributor

@OGKevin OGKevin commented Aug 4, 2020

This will allow time based replication.

Its blocked by #2906

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

Changes

  • Added TimePartitionMetaFilter for thanos replication, this means that replicate can now be time based.

Verification

@OGKevin OGKevin force-pushed the add-time-filter-to-replicate branch 2 times, most recently from 9f52fb1 to cc16483 Compare August 4, 2020 12:54
@OGKevin OGKevin force-pushed the add-time-filter-to-replicate branch from bdc6443 to 87d81a3 Compare August 5, 2020 11:49
@OGKevin OGKevin marked this pull request as ready for review August 5, 2020 11:55
cmd/thanos/tools_bucket.go Outdated Show resolved Hide resolved
@OGKevin OGKevin force-pushed the add-time-filter-to-replicate branch from d57faab to e3c36f0 Compare August 6, 2020 12:22
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice, just comments around deletion not being part of replicator

CHANGELOG.md Outdated Show resolved Hide resolved
cmd/thanos/tools_bucket.go Outdated Show resolved Hide resolved
cmd/thanos/tools_bucket.go Outdated Show resolved Hide resolved
pkg/block/block.go Outdated Show resolved Hide resolved
pkg/replicate/replicator.go Outdated Show resolved Hide resolved
pkg/replicate/replicator.go Outdated Show resolved Hide resolved
@OGKevin
Copy link
Contributor Author

OGKevin commented Aug 10, 2020

Im waiting on the depended MR to get merged to continue on this one. As the dependent MR will most likely introduce more merging conflicts.

@stale
Copy link

stale bot commented Oct 17, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Oct 17, 2020
@OGKevin
Copy link
Contributor Author

OGKevin commented Oct 17, 2020

@bwplotka can the stale label be removed? As this is blocked on #2906

@stale stale bot removed the stale label Oct 17, 2020
@kakkoyun
Copy link
Member

@yeya24 FYI this is also blocked by #2906

@kakkoyun
Copy link
Member

@OGKevin In the meanwhile, could you maybe rebase and also sign DCO?

This will allow time based replication.

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
@OGKevin OGKevin force-pushed the add-time-filter-to-replicate branch from e3c36f0 to a4d95b8 Compare October 31, 2020 11:08
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
@OGKevin OGKevin force-pushed the add-time-filter-to-replicate branch 2 times, most recently from 2d98401 to e5297d7 Compare October 31, 2020 11:20
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
@OGKevin OGKevin force-pushed the add-time-filter-to-replicate branch from e5297d7 to c18d069 Compare October 31, 2020 11:22
OGKevin and others added 2 commits October 31, 2020 12:52
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
@OGKevin
Copy link
Contributor Author

OGKevin commented Oct 31, 2020

@bwplotka its ready for 👀!

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Looks good generally, some suggestions.

Thanks 👍

CHANGELOG.md Outdated Show resolved Hide resolved
fromBkt,
"",
reg,
[]thanosblock.MetadataFilter{thanosblock.NewTimePartitionMetaFilter(*minTime, *maxTime)},
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 add this only if minTime and maxTime are passed? (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't the behaviour be the same? As the max and min are almost infinite. What is the reason for only passing it if its set to non default value?

Copy link
Member

Choose a reason for hiding this comment

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

Just no risk ;p

pkg/replicate/scheme.go Outdated Show resolved Hide resolved
pkg/replicate/scheme.go Outdated Show resolved Hide resolved
pkg/replicate/scheme.go Outdated Show resolved Hide resolved
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing! Looks good to me 👍

Thanks!

@bwplotka bwplotka merged commit 3ce844b into thanos-io:master Nov 3, 2020
@OGKevin OGKevin deleted the add-time-filter-to-replicate branch November 3, 2020 22:55
Oghenebrume50 pushed a commit to Oghenebrume50/thanos that referenced this pull request Dec 7, 2020
…io#2979)

* Add TimePartitionMetaFilter to thanos replicate.

This will allow time based replication.

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Update docs and changelog.

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Fix lint

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Add posibility to delete blocks older than max time

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Fix metric naming.

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Fix order of arguments for max and min time.

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Remove deletion of blocks from replicator.

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Fix import formatting

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Revert unneeded changes due to replicator no longer deleting blocs

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>

* Fix typo

Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com>
Signed-off-by: Oghenebrume50 <raphlbrume@gmail.com>
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.

4 participants