-
Notifications
You must be signed in to change notification settings - Fork 526
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
Add compactor HTTP API for uploading TSDB blocks #1694
Conversation
24d99f3
to
113808c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aknuds1 for working on this! I did a very high level review, focusing on API design. Could you take a look at my comments, please?
Thanks for the review @pracucci! I will incorporate your feedback when I can start refining the PR. Concentrating on implementing the endpoint for finishing backfills now. |
What happens when an upload is aborted? If the client interrupts the upload and will never recover it, the partial files will be stored in |
@pracucci I have thought about the same issue, and was planning to return to it after implementing the basic scenario ("happy path"). Good thinking about letting the compactor cleanup do it, I can note it in the design doc. |
46fa91c
to
817b5a1
Compare
What is the rationale behind using distributor, grpc and ingester to upload blocks to the storage? I would suggest to use component, which already has access to the blocks storage. Personally I would suggest |
@pstibrany it's because I figured the distributor would be the logical component for receiving the backfill requests. Would there be any (technical) drawbacks from putting the HTTP endpoints in It seems a bit weird to me from a logical PoV though to put backfill endpoints in compactor or purger components though? It's basically a type of ingestion? WDYT @pracucci? |
Distributor doesn't currently talk to blocks storage in any way.
I don't think so, other than it seems "weird". Compactor already handles lot of blocks maintanance work (cleanup, block index), but doesn't currently expose any API. "Purger" doesn't exactly mean "backfilling" in my mind. If we overlook these minor issues, I think they both are better choice than distributor. We may also end up with completely separate "backfill" module, and only people interested in having backfill API can run it. (Or "blocks-api" module, and then we can also provide blocks-reading API in the future.) |
@pstibrany It forwards ingestion requests to the ingester though. My thinking was backfill is another form of ingestion. I'm not sure what's technically the best. I'm open to moving the functionality to another component. I think compactor sounds the least weird of the two, logically.
That could also make sense, although maybe overkill for this PR? |
I think the best option is one where we can leverage HTTP protocol, so that client can stream the data through some Mimir component directly to the storage, without any intermediate local files. That rules out distributor (no access to blocks storage). Ingesters could handle the HTTP API calls directly (without going through distributors), but I don't think they should be wasting their CPU cycles on this, or on blocks verification.
Perhaps, I don't think cost of having more modules is so high. Compactors have advantage that they already have local disk (we will need that to download full block and validate it, before successfully finishing the upload). |
@pstibrany how about we move the endpoints to the compactor module then? |
Sounds good to me. It allows us to get rid of gRPC middle step. We can keep backfill implementation separate from compactor, and only pass bucket configuration to the HTTP handlers. It will make it simple to use from different module in the future. |
@pstibrany Sounds good! I'll just verify tomorrow with @pracucci that he agrees. |
We're working to simplify the Mimir architecture and operations. In this perspective, for users it's easier to understand there are only 2 ingress components in Mimir:
Do the backfill API write or read? Write. Then I would keep the API exposed by distributor. Then, internally we can route requests to other components. I'm fine routing to compactor instead of ingester (I think it makes sense), but I would keep the API exposed by distributor. This also allow for some traffic sharding (e.g. shuffle-sharding). For the same reason, I would avoid adding any new component for backfilling. About the purger: the existance of this component is a legacy of the chunks storage. I believe it shouldn't even exist in Mimir anymore (and its feature merged in other already-existing components), but that's a separate issue to discuss. |
Thanks for chiming in @pracucci! I'll keep the API as is then, considering moving the block storage logic to the compactor. |
I would like to hear @09jvilla opinion too. Exposing the API directly from compactor is easier from an implementation perspective. My main take is about simplifying the architecture for the final user, but that doesn't necessarily simplify the implementation for us. |
If it is the Distributor that exposes the API, and we don't want distributors doing the upload to the storage directly (which would be an option too, but I don't think we should do that), then distributor needs to reroute the request. We use gRPC for this rerouting. gRPC introduces extra complication and forces us to reimplement streaming of body from HTTP request via gRPC. If we didn't introduce gRPC to the mix, we could simply pass Another thing to consider: component handling the upload will need to perform validation of the block during the finish. This means downloading the block locally and perform various checks on the index and chunks, to make sure we don’t let wrong block in. I don’t think ingesters should be doing this validation – it’s IO and CPU intensive, and ingesters have better things to do. Similarly distributors would be a poor choice. I think compactor makes sense to do this. tl;dr: I'm still in favor of directly exposing upload HTTP API from compactor, without going through distributor first. @09jvilla what is your take? |
@pstibrany @pracucci I implemented Andy's/Peter's proposal to upload blocks directly to the destination, instead of going via a staging directory ("uploads"). I ensure that the block is considered unfinished while uploading by only writing meta.json when completing the block upload session. A great benefit of this is I don't have to fork Thanos to implement a bucket move method, which is actually a lot of work due to all the different bucket backends. |
Warning - long thread below. I've put in a TLDR at the bottom if you're in a hurry.
So you can still keep it relatively simple for the user, but still have this live on the compactor. TLDR: In most cases, this should be a one time action the user is taking. ("As a user, I need to spin up a new cluster and want to pull in my historic data"). There are some folks like Adobe who need to periodically import historic data, but I think this is the less common case. Because its not a super frequent action the user would take, I'm not that worried that it will add a lot of user confusion (plus I tried to make some arguments in # 1- # 4 that maybe the confusion isn't as bad as we think). Given that, I would vote in favor of what is simpler for us to maintain in the long term, which if I'm reading this thread correctly, would be putting it on the compactor. |
Thanks @09jvilla.
I'm not aware of any extra configuration required, at least none has been introduced so far in the PR.
I'd need input from the others (Marco, Peter, others?) on how much extra maintenance effort the (gRPC) hop from distributor to ingester might mean in practice. |
From user's point of view, there is no extra complexity due to using gRPC. Extra complexity is only in our code. We already have API exposed by different components (distributor, query-frontend, alertmanager, ruler), so adding compactor to the mix with backfilling API (and also perhaps tenant deletion API, which currently lives in "purger" component) doesn't seem to make the situation much worse. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@@ -30,6 +30,7 @@ | |||
* [ENHANCEMENT] Chunk Mapper: reduce memory usage of async chunk mapper. #2043 | |||
* [ENHANCEMENT] Ingesters: Added new configuration option that makes it possible for mimir ingesters to perform queries on overlapping blocks in the filesystem. Enabled with `-blocks-storage.tsdb.allow-overlapping-queries`. #2091 | |||
* [ENHANCEMENT] Ingester: reduce sleep time when reading WAL. #2098 | |||
* [ENHANCEMENT] Compactor: Add HTTP API for uploading TSDB blocks. #1694 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also mention that the feature is experimental (here: docs/sources/operators-guide/configuring/about-versioning.md:35) for now, as API may change a bit still (I expect that validation will modify "finish block" call to return progress updates).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up @pstibrany, done. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I think we're ready to merge this PR after last comments are fixed.
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your effort on this PR!
Thanks for the thorough and quick reviews @pstibrany! |
* Compactor: Add HTTP API for uploading TSDB blocks Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> Co-authored-by: Peter Štibraný <pstibrany@gmail.com> Co-authored-by: aldernero <vernon.w.miller@gmail.com>
What this PR does
Add compactor HTTP API for uploading TSDB blocks.
TODOs
minTime
/maxTime
in meta.json (@aldernero working on this)sanitizeMeta
thanos.source
property in meta.json)Which issue(s) this PR fixes or relates to
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]