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

FixHierarchyCircuitBreakerServiceTests#testG1OverLimitStrategyThrottling assertions #104963

Merged
merged 1 commit into from
Jan 31, 2024
Merged

FixHierarchyCircuitBreakerServiceTests#testG1OverLimitStrategyThrottling assertions #104963

merged 1 commit into from
Jan 31, 2024

Conversation

ldematte
Copy link
Contributor

As is, the second assertion may not always be met - hence the test failure in #104811

Extremely rare, as it requires particular thread scheduling, but possible; if there is no strategy.overLimit() invocation by the worker threads for two consecutive loop iterations of the main test thread, the number of triggered GCs (simulated) can be less than what asserted.
This can happen if we have a large-ish number of threads, and consecutive small-ish number for the CountDown latch (e.g. 1). That way the main test thread can be freed (twice) from its wait on the CountDownLatch by separate test threads that already executed strategy.overLimit() (with no trigger - they all used the time set during the previous iteration), but have not yet counted down.

Then, the test thread gets to loop twice (not stopping at the CountDownLatch), and one of the iteration goes "void" (no worker thread executes strategy.overLimit() for that iteration).

Therefore we can guarantee only that the number of triggered GCs (simulated) is only greater or equal to the previous number), not that is always progressing. I have changed the assertion this way.

An alternative that may work is to generate CountDownLatch counts to be > #threads / 2; this way we should not have 2 consecutive iterations with no invocation of strategy.overLimit(). Given that this test is to measure throttling, I thought it was better to just touch the assertion.

Closes #104811

@ldematte ldematte added >test Issues or PRs that are addressing/adding tests :Core/Infra/Core Core issues without another label labels Jan 31, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team v8.13.0 labels Jan 31, 2024
@ldematte ldematte changed the title Fix test assertions FixHierarchyCircuitBreakerServiceTests#testG1OverLimitStrategyThrottling assertions Jan 31, 2024
pgomulka
pgomulka approved these changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM

@ldematte
Copy link
Contributor Author

Thanks Przemek!

@ldematte ldematte merged commit 5013ea4 into elastic:main Jan 31, 2024
15 checks passed
@ldematte ldematte deleted the fix/tests/g1gc-strategy-throttling branch January 31, 2024 12:04
benwtrent added a commit that referenced this pull request Jan 31, 2024
@benwtrent @thecoop
@ChrisHegarty @DaveCTurner @jimczi @elasticmachine @navarone-feekery @iverase @rockdaboot @pxsalehi @masseyke @szabosteve @mosche @costin @jonathan-buttner @albertzaharovits @mark-vieira @jedrazb @dnhatn @abdonpijpelink @leemthompo @przemekwitek @joegallo @ldematte @luigidellaquila @original-brownbear @alex-spies @davidkyle
Update/main (#104974)
47ca7ae 
* Change release version lookup to an instance method (#104902)

* Upgrade to Lucene 9.9.2 (#104753)

This commit upgrades to Lucene 9.9.2.

* Improve `CANNOT_REBALANCE_CAN_ALLOCATE` explanation (#104904)

Clarify that in this situation there is a rebalancing move that would
improve the cluster balance, but there's some reason why rebalancing is
not happening. Also points at the `can_rebalance_cluster_decisions` as
well as the node-by-node decisions since the action needed could be
described in either place.

* Get from translog fails with large dense_vector (#104700)

This change fixes the engine to apply the current codec when retrieving documents from the translog.
We need to use the same codec than the main index in order to ensure that all the source data is indexable.
The internal codec treats some fields differently than the default one, for instance dense_vectors are limited to 1024 dimensions.
This PR ensures that these customizations are applied when indexing document for translog retrieval.

Closes #104639

Co-authored-by: Elastic Machine <[email protected]>

* [Connector Secrets] Add delete API endpoint (#104815)

* Add DELETE endpoint for /_connector/_secret/{id}
* Add endpoint to write_connector_secrets cluster privilege

* Merge Aggregations into InternalAggregations (#104896)

This commit merges Aggregations into InternalAggregations in order to remove the unnecessary hierarchy.

* [Profiling] Simplify cost calculation (#104816)

* [Profiling] Add the number of cores to HostMetadata

* Update AWS pricelist (remove cost_factor, add usd_per_hour)

* Switch cost calculations from 'cost_factor' to 'usd_per_hour'

* Remove superfluous CostEntry.toXContent()

* Check for Number type in CostEntry.fromSource()

* Add comment

* Retry get_from_translog during relocations (#104579)

During a promotable relocation, a `get_from_translog` sent by the
unpromotable  shard to handle a real-time get might encounter
`ShardNotFoundException` or  `IndexNotFoundException`. In these cases,
we should retry.

This is just for `GET`. I'll open a second PR for `mGET`.  The relevant
IT is in the  Stateless PR.

Relates ES-5727

* indicating fix for 8.12.1 for int8_hnsw (#104912)

* Removing the assumption from some tests that the request builder's request() method always returns the same object (#104881)

* [DOCS] Adds get setting and update settings asciidoc files to security API index (#104916)

* [DOCS] Adds get setting and update settings asciidoc files to security API index.

* [DOCS] Fixes references in docs.

* Reuse APMMeterService of APMTelemetryProvider (#104906)

* Mute more tests that tend to leak searchhits (#104922)

* ESQL: Fix SearchStats#count(String) to count values not rows (#104891)

SearchStats#count incorrectly counts the number of documents (or rows)
 in which a document appears instead of the actual number of values.
This PR fixes this by looking at the term frequency instead of the doc
 count.

Fix #104795

* Adding request source for cohere (#104926)

* Fixing a broken javadoc comment in ReindexDocumentationIT (#104930)

This fixes a javadoc comment that was broken by #104881

* Fix enabling / disabling of APM agent "recording" in APMAgentSettings (#104324)

* Add `type` parameter support, for sorting, to the Query API Key API (#104625)

This adds support for the `type` parameter, for sorting, to the Query API key API.
The type for an API Key can currently be either `rest` or `cross_cluster`.
This was overlooked in #103695 when support for the `type` parameter
was first introduced only for querying.

* Apply publish plugin to es-opensaml-security-api project (#104933)

* Support `match` for the Query API Key API (#104594)

This adds support for the `match` query type to the Query API key Information API.
Note that since string values associated to API Keys are mapped as `keywords`,
a `match` query with no analyzer parameter is effectively equivalent to a `term` query
for such fields (e.g. `name`, `username`, `realm_name`).

Relates: #101691

* [Connectors API] Relax strict response parsing for get/list operations (#104909)

* Limit concurrent shards per node for ESQL (#104832)

Today, we allow ESQL to execute against an unlimited number of shards 
concurrently on each node. This can lead to cases where we open and hold
too many shards, equivalent to opening too many file descriptors or
using too much memory for FieldInfos in ValuesSourceReaderOperator.

This change limits the number of concurrent shards to 10 per node. This 
number was chosen based on the _search API, which limits it to 5.
Besides the primary reason stated above, this change has other
implications:

We might execute fewer shards for queries with LIMIT only, leading to 
scenarios where we execute only some high-priority shards then stop. 
For now, we don't have a partial reduce at the node level, but if we
introduce one in the future, it might not be as efficient as executing
all shards at the same time.  There are pauses between batches because
batches are executed sequentially one by one.  However, I believe the
performance of queries executing against many shards (after can_match)
is less important than resiliency.

Closes #103666

* [DOCS] Support for nested functions in ES|QL STATS...BY (#104788)

* Document nested expressions for stats

* More docs

* Apply suggestions from review

- count-distinct.asciidoc
  - Content restructured, moving the section about approximate counts to end of doc.

- count.asciidoc
  - Clarified that omitting the `expression` parameter in `COUNT` is equivalent to `COUNT(*)`, which counts the number of rows.

- percentile.asciidoc
  - Moved the note about `PERCENTILE` being approximate and non-deterministic to end of doc.

- stats.asciidoc
  - Clarified the `STATS` command
  -  Added a note indicating that individual `null` values are skipped during aggregation

* Comment out mentioning a buggy behavior

* Update sum with inline function example, update test file

* Fix typo

* Delete line

* Simplify wording

* Fix conflict fix typo

---------

Co-authored-by: Liam Thompson <[email protected]>
Co-authored-by: Liam Thompson <[email protected]>

* [ML] Passing input type through to cohere request (#104781)

* Pushing input type through to cohere request

* switching logic to allow request to always override

* Fixing failure

* Removing getModelId calls

* Addressing feedback

* Switching to enumset

* [Transform] Unmute 2 remaining continuous tests: HistogramGroupByIT and TermsGroupByIT (#104898)

* Adding ActionRequestLazyBuilder implementation of RequestBuilder (#104927)

This introduces a second implementation of RequestBuilder (#104778). As opposed
to ActionRequestBuilder, ActionRequestLazyBuilder does not create its request
until the request() method is called, and does not hold onto that request (so each
call to request() gets a new request instance).
This PR also updates BulkRequestBuilder to inherit from ActionRequestLazyBuilder
as an example of its use.

* Update versions to skip after backport to 8.12 (#104953)

* Update/Cleanup references to old tracing.apm.* legacy settings in favor of the telemetry.* settings (#104917)

* Exclude tests that do not work in a mixed cluster scenario (#104935)

* ES|QL: Improve type validation in aggs for UNSIGNED_LONG and better support for VERSION (#104911)

* [Connector API] Make update configuration action non-additive (#104615)

* Save allocating enum values array in two hot spots (#104952)

Our readEnum code instantiates/clones enum value arrays on read.
Normally, this doesn't matter much but the two spots adjusted here are
visibly hot during bulk indexing, causing GBs of allocations during e.g.
the http_logs indexing run.

* ESQL: Correct out-of-range filter pushdowns (#99961)

Fix pushed down filters for binary comparisons that compare a
byte/short/int/long with an out of range value, like
WHERE some_int_field < 1E300.

* [DOCS] Dense vector element type should be float for OpenAI (#104966)

* Fix test assertions (#104963)

* Move functions that generate lucene geometries under a utility class (#104928)

We have functions that generate lucene geometries scattered in different places of the code. This commit moves 
everything under a utility class.

* fixing index versions

---------

Co-authored-by: Simon Cooper <[email protected]>
Co-authored-by: Chris Hegarty <[email protected]>
Co-authored-by: David Turner <[email protected]>
Co-authored-by: Jim Ferenczi <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Navarone Feekery <[email protected]>
Co-authored-by: Ignacio Vera <[email protected]>
Co-authored-by: Tim Rühsen <[email protected]>
Co-authored-by: Pooya Salehi <[email protected]>
Co-authored-by: Keith Massey <[email protected]>
Co-authored-by: István Zoltán Szabó <[email protected]>
Co-authored-by: Moritz Mack <[email protected]>
Co-authored-by: Costin Leau <[email protected]>
Co-authored-by: Jonathan Buttner <[email protected]>
Co-authored-by: Albert Zaharovits <[email protected]>
Co-authored-by: Mark Vieira <[email protected]>
Co-authored-by: Jedr Blaszyk <[email protected]>
Co-authored-by: Nhat Nguyen <[email protected]>
Co-authored-by: Abdon Pijpelink <[email protected]>
Co-authored-by: Liam Thompson <[email protected]>
Co-authored-by: Liam Thompson <[email protected]>
Co-authored-by: Przemysław Witek <[email protected]>
Co-authored-by: Joe Gallo <[email protected]>
Co-authored-by: Lorenzo Dematté <[email protected]>
Co-authored-by: Luigi Dell'Aquila <[email protected]>
Co-authored-by: Armin Braun <[email protected]>
Co-authored-by: Alexander Spies <[email protected]>
Co-authored-by: David Kyle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgomulka pgomulka pgomulka approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI] HierarchyCircuitBreakerServiceTests testG1OverLimitStrategyThrottling failing
3 participants
@ldematte @elasticsearchmachine @pgomulka