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 hppc from task manager #85889

Merged
merged 1 commit into from
Apr 18, 2022
Merged

Remove hppc from task manager #85889

merged 1 commit into from
Apr 18, 2022

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Apr 14, 2022

The task manager uses hppc to keep track of the count of child tasks for
each network connection. This is bookkeeping code and not performance
critical, nor is it memory intensive (the map won't be that large). This
commit converts the code to use a HashMap.

relates #84735

The task manager uses hppc to keep track of the count of child tasks for
each network connection. This is bookkeeping code and not performance
critical, nor is it memory intensive (the map won't be that large). This
commit converts the code to use a HashMap.

relates elastic#84735
@rjernst rjernst added :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. >refactoring v8.3.0 labels Apr 14, 2022
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Apr 14, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@rjernst
Copy link
Member Author

rjernst commented Apr 14, 2022

@elasticmachine run elasticsearch-ci/part-2

@rjernst rjernst mentioned this pull request Apr 14, 2022
43 tasks
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

This looks good to me! The only issue I see is that TaskManagerTests has no coverage for the register and unregister methods, where we bump and decrease the task counts. Do we have other tests that would validate the code?

@rjernst
Copy link
Member Author

rjernst commented Apr 18, 2022

@grcevski You are right TaskManagerTests does not explicitly call unregister (though it does call register). The task framework is used indirectly by a lot of tests. I agree It would be much better to have a unit test of this functionality, but these tests are a bit complex right now, and this change is just a trivial type change. I would like to merge, and will look to refactor these tests for better direct coverage in the future. wdyt?

Copy link
Contributor

@grcevski grcevski 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 agree, I'll take on the task to add some tests for this code.

@rjernst rjernst merged commit 580a5bc into elastic:master Apr 18, 2022
@rjernst rjernst deleted the hppc/tasks branch April 18, 2022 15:50
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Apr 21, 2022
* master: (104 commits)
  fix: ordering terms aggregation on top metrics null values (elastic#85774)
  Fix up whitespace error introduced in elastic#85948
  More docs re. removing cluster.initial_master_nodes (elastic#85948)
  [Test] Remove API key methods from HLRC (elastic#85802)
  Remove references to bootstrap.system_call_filter (elastic#85964)
  Move docker cgroup override to SystemJvmOptions (elastic#85960)
  Add connection accounting tests (elastic#85966)
  Remove MacOS from platform support testing matrix
  Remove custom KnnVectorFieldExistsQuery (elastic#85945)
  Relax data path deprecations from critical to warn (elastic#85952)
  Remove hppc from some "common" classes (elastic#85957)
  Move docker env var settings handling out of bash (elastic#85913)
  Remove hppc from task manager (elastic#85889)
  [ML] rename trained model allocations to assignments (elastic#85503)
  Remove hppc from multi*shard request and responses (elastic#85888)
  Consolidating logging initialization in cli launcher (elastic#85920)
  Convert license tools to use unified cli entrypoint (elastic#85919)
  Add noop detection to node shutdown actions (elastic#85914)
  Adjust SQL expended test output
  TSDB: Add timestamp provider to AggregationExecutionContext (elastic#85850)
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionContext.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. >refactoring Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants