Skip to content

Conversation

@gwbrown
Copy link
Contributor

@gwbrown gwbrown commented Apr 12, 2019

Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for this class, rather than using the
(deprecated) inherited one.

Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for this class, rather than using the
(deprecated) inherited one.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, I left one suggestion

new AckedClusterStateUpdateTask<Response>(request, listener) {
@Override
public ClusterState execute(ClusterState currentState) {
logger.info("moving index [{}] from [{}] to [{}]",
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible for us to pull the policy name out here too and put it in? I think that might be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to do this I needed to move the log statement to IndexLifecycleRuner#moveClusterStateToStep as we don't know the policy in the transport action. However, the javadoc for that method says that it's intended for moving steps in response to TransportAction executions, so I think it's fine to be there instead. That also lets us emit the logs after other checks (e.g. making sure the index is actually managed by any policy at all).

Copy link
Member

Choose a reason for hiding this comment

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

That's going to log a lot more often, since that's called for almost every step move, is that what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IndexLifecycleRuner#moveClusterStateToStep is only called via TransportMoveToStepAction and TransportRetryAction, I think you're thinking of IndexLifecycleRunner#moveClusterStateToNextStep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These method names could use a bit of work.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, they definitely could, okay, I'm good with this where it is then :)

@gwbrown gwbrown merged commit de825d2 into elastic:master Apr 15, 2019
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Apr 15, 2019
Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for TransportMoveToStepAction, 
rather than using the (deprecated) inherited one.
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Apr 15, 2019
Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for TransportMoveToStepAction, 
rather than using the (deprecated) inherited one.
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Apr 15, 2019
Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for TransportMoveToStepAction, 
rather than using the (deprecated) inherited one.
gwbrown added a commit that referenced this pull request Apr 15, 2019
Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for TransportMoveToStepAction, 
rather than using the (deprecated) inherited one.
gwbrown added a commit that referenced this pull request Apr 15, 2019
Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for TransportMoveToStepAction, 
rather than using the (deprecated) inherited one.
gwbrown added a commit that referenced this pull request Apr 15, 2019
Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for TransportMoveToStepAction, 
rather than using the (deprecated) inherited one.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
Usage of the ILM Move to Step API can result in some very odd
situations, and for diagnosing problems arising from these situations it
would be nice to have a record of when this API was called with what
parameters.

Also, adds a dedicated logger for TransportMoveToStepAction, 
rather than using the (deprecated) inherited one.
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.

4 participants