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

Deprecate undocumented alternatives to the nodes hot threads API (#52640) #52930

Merged
merged 20 commits into from
Mar 24, 2020
Merged

Deprecate undocumented alternatives to the nodes hot threads API (#52640) #52930

merged 20 commits into from
Mar 24, 2020

Conversation

muachilin
Copy link
Contributor

@muachilin muachilin commented Feb 27, 2020 •
edited
Loading

This PR deprecates the undocumented alternatives to the nodes hot threads API.

[issue: #52640]

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 27, 2020 •
edited
Loading

💚 CLA has been signed

@muachilin
Copy link
Contributor Author

I have signed the Contribution Agreement but I still cannot pass the checks

jasontedor
jasontedor requested changes Feb 27, 2020
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up. I left a comment.

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java Outdated Show resolved Hide resolved
@muachilin muachilin mentioned this pull request Mar 2, 2020
Closed
jasontedor
jasontedor reviewed Mar 2, 2020
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left a question.

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java Outdated Show resolved Hide resolved
jasontedor
jasontedor requested changes Mar 2, 2020
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left a few more comments, and also the comment about the deprecated route lookup in the prepareRequest method.

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java Outdated Show resolved Hide resolved
jasontedor
jasontedor reviewed Mar 11, 2020
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left some comments.

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java Outdated Show resolved Hide resolved
@matriv matriv added the :Core/Infra/REST API REST infrastructure and utilities label Mar 11, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/REST API)

jasontedor
jasontedor requested changes Mar 13, 2020
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

Thanks, we're getting there now. One more thing, and also can you please sign the CLA?

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java Outdated
public class RestNodesHotThreadsAction extends BaseRestHandler {

static final String formatDeprecatedMessageWithoutNodeID = "[%s] is a deprecated endpoint. Please use [/nodes/hot_threads] instead.";
Copy link
Member

Choose a reason for hiding this comment

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

These can all be private now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have signed the CLA but I did not get any confirmation email or other notification to indicate that I have successfully signed it. Thanks!

@jasontedor
Copy link
Member

cla/check

@jasontedor
Copy link
Member

@elasticmachine update branch

@elasticmachine test this please

@jasontedor
Copy link
Member

@elasticmachine update branch

@muachilin
Copy link
Contributor Author

Okay! Thank you.

@jasontedor
Copy link
Member

@elasticmachine test this please

@muachilin
Copy link
Contributor Author

@elasticmachine update branch

@jasontedor
Copy link
Member

@muachilin Only to let you know, only maintainers can trigger CI test runs (as you saw, you can use the bot to update the branch with the latest changes from the target branch though).

@jasontedor
Copy link
Member

@elasticmachine test this please

@muachilin
Copy link
Contributor Author

I see. This is my first time to handle issues in elastic's OSS. Thanks for your time! 😊

@jasontedor
Copy link
Member

@elasticmachine update branch

@jasontedor
Copy link
Member

@elasticmachine test this please

jasontedor
jasontedor requested changes Mar 19, 2020
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

Okay, I was getting ready to merge this in and I noticed one issue. Can you address?

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java Outdated Show resolved Hide resolved
@muachilin
Copy link
Contributor Author

@elasticmachine update branch

@jasontedor
Copy link
Member

@elasticmachine test this please

jasontedor
jasontedor approved these changes Mar 24, 2020
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM!

@muachilin
Copy link
Contributor Author

Thanks!

@jasontedor jasontedor merged commit 7989d67 into elastic:master Mar 24, 2020
jasontedor pushed a commit that referenced this pull request Mar 24, 2020
@muachilin @jasontedor
Deprecate alternatives to the hot threads API (#52930)
b33fbe7 
This commit deprecates various undocumented alternatives to the hot
threads API.
@jasontedor
Copy link
Member

Thanks @muachilin!

@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@changjenyin changjenyin changjenyin left review comments

@jasontedor jasontedor jasontedor approved these changes

Assignees
No one assigned
Labels
:Core/Infra/REST API REST infrastructure and utilities >deprecation v7.7.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@muachilin @elasticmachine @jasontedor @changjenyin @jakelandis @matriv