Skip to content

add documentation for coordinator dynamic configuration#11052

Merged
clintropolis merged 1 commit intoapache:masterfrom
clintropolis:coordinator-dynamic-config-docs
Apr 3, 2021
Merged

add documentation for coordinator dynamic configuration#11052
clintropolis merged 1 commit intoapache:masterfrom
clintropolis:coordinator-dynamic-config-docs

Conversation

@clintropolis
Copy link
Copy Markdown
Member

Docs were missing, also fixed an error in overlord dynamic config example


* `/druid/coordinator/v1/config`

Update overlord dynamic worker configuration.
Copy link
Copy Markdown
Contributor

@techdocsmith techdocsmith Mar 30, 2021

Choose a reason for hiding this comment

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

Seems odd not to have the payload here, but I see that it is in the linked section. Might be worth a mention to get the payload there?

Copy link
Copy Markdown
Contributor

@capistrant capistrant Mar 31, 2021

Choose a reason for hiding this comment

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

I'm indifferent on this note. Since other POST endpoints in this same doc do not include information on what the payload would look like, I think it is fine as is. (this is a reply to another review comment: #11052 (comment))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @capistrant ! I think you are right, too. Everything API related in Druid could use an overhaul to be a little more user-friendly and that surely wasn't the goal of this PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah yeah, I just copied this from the overlord section. It would be really nice to have example requests and responses for all of these API calls, but I think maybe that could be done as future work.


* `/druid/coordinator/v1/config`

Update overlord dynamic worker configuration.
Copy link
Copy Markdown
Contributor

@capistrant capistrant Mar 31, 2021

Choose a reason for hiding this comment

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

I'm indifferent on this note. Since other POST endpoints in this same doc do not include information on what the payload would look like, I think it is fine as is. (this is a reply to another review comment: #11052 (comment))

@capistrant
Copy link
Copy Markdown
Contributor

I see that console is having some CI issues on this and another docs related PR, #11053. The docs job within CI passed for both though. Haven't had a chance to cross check these CI stages with other PRs to see if they are broken everywhere.

@clintropolis
Copy link
Copy Markdown
Member Author

I see that console is having some CI issues on this and another docs related PR, #11053. The docs job within CI passed for both though. Haven't had a chance to cross check these CI stages with other PRs to see if they are broken everywhere.

I think it was some sort of strange travis caching issue, since I saw it on some non doc related PRs too. I think I resolved it by side-effect with #11057 (seemed easier to do this than hunt down and delete all the caches in travis), so when we retry CI it should be working now. I restarted on this PR just for fun to test this theory.

@clintropolis clintropolis merged commit 470d659 into apache:master Apr 3, 2021
@clintropolis clintropolis deleted the coordinator-dynamic-config-docs branch April 3, 2021 05:01
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants