Skip to content

Conversation

@hub-cap
Copy link
Contributor

@hub-cap hub-cap commented Aug 22, 2019

When a policy is deleted, the enrich indices that are backing the policy
alias should also be deleted. This commit does that work and cleans up
the transport action a bit so that the lock release is easier to see, as
well as to ensure that any action carried out, regardless of exception,
unlocks the policy.

When a policy is deleted, the enrich indices that are backing the policy
alias should also be deleted. This commit does that work and cleans up
the transport action a bit so that the lock release is easier to see, as
well as to ensure that any action carried out, regardless of exception,
unlocks the policy.
@hub-cap hub-cap added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Aug 22, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

Copy link
Member

@jbaiera jbaiera left a comment

Choose a reason for hiding this comment

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

LGTM Left a quick blurb about being apprehensive about using locks outside of a try-catch-finally mechanism but I think it might be out of scope for this PR

if (policy == null) {
throw new ResourceNotFoundException("policy [{}] not found", request.getName());
}
enrichPolicyLocks.lockPolicy(request.getName());
Copy link
Member

Choose a reason for hiding this comment

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

I know that this line isn't from this PR but I'm wondering if it makes sense if we can get this to unlock from within a catch or finally block? I'm mostly paranoid about some benign exception being thrown anywhere that isn't expected and the policy becoming locked forever. Might require some restructuring here though and maybe not appropriate for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be quite simple to do in this PR. I already had to add release logic to this PR, so i can just move it and remove a bit of code in the process.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jbaiera jbaiera left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making those locking changes

@hub-cap hub-cap merged commit 1c4ffd3 into elastic:enrich Aug 23, 2019
@hub-cap hub-cap deleted the enrich_delete_index branch August 23, 2019 19:34
hub-cap added a commit that referenced this pull request Aug 23, 2019
When a policy is deleted, the enrich indices that are backing the policy
alias should also be deleted. This commit does that work and cleans up
the transport action a bit so that the lock release is easier to see, as
well as to ensure that any action carried out, regardless of exception,
unlocks the policy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants