thanos
thanos copied to clipboard
Receiver: Use proxy store for MultiTSDB
- [ ] I added CHANGELOG entry for this change.
- [ ] Change is not relevant to the end user.
Changes
This PR attempts to address a long-standing issue https://github.com/thanos-io/thanos/issues/2864.
The PR proposes a couple of changes for the solution to work:
- It adjusts the
Clientinterface to replaceAddrmethod with methodStoreInfo. The aim of this is to accommodate for both cases of clients - remote clients (i.e. the typical store client that proxy is dialing remotely) and local clients (this are clients where proxy won't be making an external call - so in our case theMultiTSDB- but rather we'll use obtain the series 'locally' from individual tenant TSDBs).StoreInfoshall return information about store client, namely: a) is this a remote or local client; b) if this client is remote, return it's address as well. Just as previously, these information can be then used e.g. for logging and debugging store connections. - Add
localClientimplementation ofproxy.Clientto MultiTSDB. - Use
ServerAsClientimplementation of theStoreClientgRPC interface, to obtain series. This shall replace the existing snowflake solution with an implementation ofproxy.Client.
Verification
Test have been adjust and are running succesfully.
I have also done a few rounds with the interactive test and everything seems to be working as expected.
I'd welcome feedback on the idea of local / remote client (whether this is a good adjustment to the interface) as well as feedback on using server-as-client, whether there are any drawbacks or potential issues with using this store client implementation (I don't think it's used anywhere besides testing).
@matej-g would you mind rebasing this PR? I'd like to do another review.
@matej-g would you mind rebasing this PR? I'd like to do another review.
Phew, done, thanks for taking a look! :slightly_smiling_face:
Hey @fpetkovski @bwplotka,
Thanks for the reviews, I addressed all of the comments, except for using lazy retrieval strategy in proxy, since it seem to be causing receiver to panic. If we're happy to move forward with eager strategy for now, I can take a look and figure it out on the side.
I'll wait on https://github.com/thanos-io/thanos/pull/5717 and switch the eval strategy, I believe then this is finally good to go :+1:
Could you please double check if https://github.com/thanos-io/thanos/pull/5746 fixes the nil panic?
This seems like to be closed accidentally by Github, reopen now.
Sorry for the delay, PR is now updated, this should be good to go once CI passes
The check for Go build for different platform on the PR is not getting out of Expected state, but the job actually ran succesfully (https://github.com/thanos-io/thanos/actions/runs/3195252383/jobs/5215692706).
I'm not sure if this is due to yesterday's webhook issues on GH side, but given this is approved and the checks actually ran successfully, I'm merging.