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.

Sign up for GitHub

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

Jump to bottom

Compactor support time filter (#4884) #4909

Merged
merged 1 commit into from
Dec 12, 2021
Merged

Compactor support time filter (#4884) #4909

merged 1 commit into from
Dec 12, 2021

Conversation

jordy1024
Copy link
Contributor

@jordy1024 jordy1024 commented Nov 27, 2021 •
edited
Loading

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

Changes

Verification

@metonymic-smokey
Copy link
Contributor

metonymic-smokey commented Nov 27, 2021 •
edited
Loading

Looks good to me(I'm not an official reviewer though).
Maybe just check once if the compact tests require any additions, since some of them invoke the other filters in compact so I think this should be added to those tests too?

@metonymic-smokey
Copy link
Contributor

@jordy1024 also to fix the failing docs test, please run make docs locally and push the resulting changes.

@jordy1024
Copy link
Contributor Author

@jordy1024 also to fix the failing docs test, please run make docs locally and push the resulting changes.

@metonymic-smokey Thanks for reminding me,I will fix it right away.

@jordy1024 jordy1024 force-pushed the issue-4884 branch 2 times, most recently from b704b72 to faaa9b3 Compare November 29, 2021 12:39
@jordy1024
Copy link
Contributor Author

Hello,metonymic-smokey @metonymic-smokey
what should I do?
The CI process is blocked every time in the Documentation check step, and the error message shows
error: mixin/README.md:186: "https://www.prometheus.io/docs/prometheus/latest/configuration/unit_testing_rules/" not accessible; status code 400: Bad Request , but the official prometheus document link is actually accessible.

@metonymic-smokey
Copy link
Contributor

@jordy1024 I faced the same issue in one of my PRs today.
As far as I understand, since that file is not related to your changes, the only way out is to maybe have a maintainer re-trigger the tests.

@jordy1024
Copy link
Contributor Author

@metonymic-smokey
I think so too, thank you very much.

@metonymic-smokey
Copy link
Contributor

metonymic-smokey commented Dec 1, 2021 •
edited
Loading

@jordy1024 np at all!
You can find me on the #thanos slack as 'Aditi Ahuja'.

yeya24
yeya24 reviewed Dec 1, 2021
View reviewed changes
cmd/thanos/compact.go Outdated
@@ -737,6 +741,14 @@ func (cc *compactConfig) registerFlag(cmd extkingpin.FlagClause) {
cmd.Flag("hash-func", "Specify which hash function to use when calculating the hashes of produced files. If no function has been specified, it does not happen. This permits avoiding downloading some files twice albeit at some performance cost. Possible values are: \"\", \"SHA256\".").
Default("").EnumVar(&cc.hashFunc, "SHA256", "")

cc.filterConf = &store.FilterConfig{}

cmd.Flag("min-time", "Start of time range limit to serve. Thanos Compactor will serve only metrics, which happened later than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y.").
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the description Thanos Compactor will serve only metrics. serve is not a suitable verb for compactor.

cmd/thanos/compact.go Outdated Show resolved Hide resolved
@jordy1024 jordy1024 force-pushed the issue-4884 branch 3 times, most recently from 6126e94 to cf2c789 Compare December 7, 2021 13:45
@jordy1024
Copy link
Contributor Author

@yeya24 Maybe it's better, use compact instead of serve and blocks instead of metrics.
Please help to review again and unblock CI.

@jordy1024
Copy link
Contributor Author

I'm not sure if it is my problem. An error occurred in Phase go / Thanos end-to-end tests (pull_request):

manifest for quay.io/prometheus/busybox@sha256:de4af55df1f648a334e16437c550a2907e0aed4f0b0edf454b0b215a9349bdbb not found: manifest unknown: manifest unknown
Error: make: *** [Makefile:155: docker] Error 1
Error: Process completed with exit code 2.

what should I do?

@yeya24
Copy link
Contributor

yeya24 commented Dec 11, 2021

I'm not sure if it is my problem. An error occurred in Phase go / Thanos end-to-end tests (pull_request):

manifest for quay.io/prometheus/busybox@sha256:de4af55df1f648a334e16437c550a2907e0aed4f0b0edf454b0b215a9349bdbb not found: manifest unknown: manifest unknown
Error: make: *** [Makefile:155: docker] Error 1
Error: Process completed with exit code 2.

what should I do?

Can you please do a rebase on main branch? I think the fix is on the main branch already

yeya24
yeya24 previously approved these changes Dec 12, 2021
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

@jordy1024 This looks good now. Can you please update the changelog?

@jordy1024 jordy1024 closed this Dec 12, 2021
@jordy1024
Copy link
Contributor Author

@jordy1024 This looks good now. Can you please update the changelog?

Sorry,it was an oversight on my part. I will complete it right away. @yeya24

@jordy1024 jordy1024 reopened this Dec 12, 2021
Compactor support time filter (thanos-io#4884)
a5fde80 
Signed-off-by: wenmaoba <wenmaoba@tencent.com>
@jordy1024 jordy1024 closed this Dec 12, 2021
@jordy1024 jordy1024 deleted the issue-4884 branch December 12, 2021 13:50
@jordy1024 jordy1024 reopened this Dec 12, 2021
yeya24
yeya24 approved these changes Dec 12, 2021
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM

@yeya24 yeya24 merged commit 9a26b0e into thanos-io:main Dec 12, 2021
@yeya24 yeya24 mentioned this pull request Dec 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeya24 yeya24 yeya24 approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jordy1024 @metonymic-smokey @yeya24