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

Remove unneeded locks in add_kubernetes_metadata #16979

Merged
merged 2 commits into from
Mar 12, 2020

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Mar 12, 2020

What does this PR do?

Read/write mutexes in matchers and indexers are only locked for read,
making them unnecesary. We could consider to add write locks, but writes
only happen on constructions, so it wouldn't be expected to read from these
structs before they are returned by the constructor.
These locks are acquired for any event that is enriched by the processor, so
unneeded locking may affect performance.

Also, refactor locking in indexing registry so it is used in all operations. It is
currently used only in some operations, and in external calls the lock needs to
be acquired in some cases.
This lock could be also removed because we only write to this structure
on init()s, but we use to make our registries thread-safe, so let's keep it
as is. It is only used when seting up the processor, and not when processing
events, so it shouldn't affect performance.

Why is it important?

Unneeded locking can be confusing and may affect performance.
Inconsistent locking may lead to unexpected results in future changes.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works (behaviour should be the same, covered by current cases)

Read/write mutexes in matchers and indexers are only locked for read,
making them unnecesary. These locks are acquired for any event that is
enriched by the processor, so unneeded locking may affect performance.

Also, refactor locking in indexing registry so it is used in all cases.
@jsoriano jsoriano added review refactoring [zube]: In Review Team:Platforms Label for the Integrations - Platforms team labels Mar 12, 2020
@jsoriano jsoriano self-assigned this Mar 12, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good to me, but probably better to get another reviewer for +1.

@jsoriano jsoriano merged commit 2a643c9 into elastic:master Mar 12, 2020
@jsoriano jsoriano deleted the add-kubernetes-metadata-locks branch March 12, 2020 16:39
jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 12, 2020
Read/write mutexes in matchers and indexers are only locked for read,
making them unnecesary. These locks are acquired for any event that is
enriched by the processor, so unneeded locking may affect performance.

Also, refactor locking in indexing registry so it is used in all cases.

(cherry picked from commit 2a643c9)
jsoriano added a commit that referenced this pull request Mar 24, 2020
Read/write mutexes in matchers and indexers are only locked for read,
making them unnecesary. These locks are acquired for any event that is
enriched by the processor, so unneeded locking may affect performance.

Also, refactor locking in indexing registry so it is used in all cases.

(cherry picked from commit 2a643c9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring review Team:Platforms Label for the Integrations - Platforms team v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants