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

receive: Add ability to consume contents of the hashrings file directly #3121

Merged
merged 5 commits into from
Dec 28, 2020

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Sep 3, 2020

Signed-off-by: Kemal Akkoyun [email protected]

  • I added CHANGELOG entry for this change.

Fixes #3026.

Changes

  • Added --receive.hashrings alternative to receive.hashrings-file flag (lower priority). Content of JSON file that contains the hashring configuration.

Verification

  • make test-local
  • make test-e2e

@kakkoyun kakkoyun changed the title receive: Add ability to consume content of the hashring directly receive: Add ability to consume contents of the hashring file directly Sep 3, 2020
@kakkoyun kakkoyun changed the title receive: Add ability to consume contents of the hashring file directly receive: Add ability to consume contents of the hashrings file directly Sep 3, 2020
cmd/thanos/receive.go Outdated Show resolved Hide resolved
@squat
Copy link
Member

squat commented Sep 3, 2020

Looking forward to taking a close look at this 👍

@kakkoyun
Copy link
Member Author

Friendly ping @squat
Also somewhat related #3138

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.

Thanks! LGTM 💪 perfect.

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.

LGTM from my side.

pkg/extflag/pathorcontent.go Outdated Show resolved Hide resolved
cmd/thanos/receive.go Outdated Show resolved Hide resolved
@kakkoyun kakkoyun force-pushed the hashring_file branch 2 times, most recently from 7e21dfd to 81dbb29 Compare December 9, 2020 13:43
@kakkoyun
Copy link
Member Author

kakkoyun commented Dec 9, 2020

@squat Thanks for the great review as always. I have addressed your concerns, could please take another look at it?

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.

Love it especially extra tests. But I think we can do better on pathOrContent API 🤗 Let's do it properly as it's quite flexible flag, so would be nice to have clean API.

}, func(error) {
cancel()
})
} else {
// The Hashrings config file path is not given, so initialize using content..
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The Hashrings config file path is not given, so initialize using content..
// The Hashrings config file path is not given, so initialize using content.

// Path returns the path of the given file that has passed to the flag.
// It returns error if both a path and content are given.
// It returns error if the required flag is set to true and path is empty.
func (p *PathOrContent) Path() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmmmm...

So Content() is a bit different semantically. We aim for Content to be always there unless flag is optional.

With Path.. we have one more case, of user not specifying path at al, but specifying content. While technically this method should be understandable there are so many cases to tests to check which of those 6 cases we have (content passed, path passed, filag not used) x2 (required or not)

While all of this we have also this important message that is hidden in terms of hashring file:

IF you specify via content it will be used. If specify via path it will be dynamically reloadable. There are 2 problems with this:

  1. This is surprising. We never reload content. How user can know we do it now? And let's say we are explicit we reload hashring. How user will know that other flags then hashrings are not dynamically reloadable?
  2. What if I want via path but don't want to dynamically reload? For example I want to put file, start receive and remove (for some reason). Maybe rare, but someone could be surprised this is not possible. Might other usecases as well.

I think we need to think this through. I have some ideas, but before I state them, maybe you have as well? 🤗 Do you see the problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is surprising. We never reload content. How user can know we do it now? And let's say we are explicit we reload hashring. How user will know that other flags then hashrings are not dynamically reloadable?

I understand the concern. Maybe overloading the PathOrContent for the Receive's use case is not the best way.
Receive has a special way to handle the given path as you already mentioned. It initializes a watcher for the given path.

What if I want via path but don't want to dynamically reload? For example I want to put file, start receive and remove (for some reason). Maybe rare, but someone could be surprised this is not possible. Might other usecases as well.

This is a problem we already have. This PR shouldn't be aiming to solve this. However, I can add more documentation to clarify this behaviour.

How about rather than overloading the PathOrContent, we implement dedicated logic in the receive. Specific to receive? Would this work? WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

hm what about, creating a second constructor?

So we can have:

  • something or something-file as one constructor
  • something and something-file and something-reloadable-file if you use another constructor

So receive will use latter (: Then we can actually embed file watching in the second constructor, directly in pathOrContent package 🤗

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea. I would love to work on it on a separate PR to introduce this feature in a more generalized form.
How about merging this PR as is and then creating an issue for it? As I told you I can work on it. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bwplotka As we talked off-line, do we have a consensus? Can we proceed with this?

Copy link
Member

Choose a reason for hiding this comment

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

YES! love it. Thanks!

Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>
Simplify hasring configuration

Signed-off-by: Kemal Akkoyun <[email protected]>
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.

Well done with short term thing 👍🏽

It will have the same flag spec 🤗

@bwplotka bwplotka merged commit dd670a3 into thanos-io:master Dec 28, 2020
@kakkoyun kakkoyun deleted the hashring_file branch January 4, 2021 10:39
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.

receive: Use PathOrContent file for the hashring
3 participants