Skip to content

More verbose logging in IndicesSegmentsRestCancellationIT #113844

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
Merged
merged 6 commits into from
Oct 3, 2024
Merged

More verbose logging in IndicesSegmentsRestCancellationIT #113844

merged 6 commits into from
Oct 3, 2024

Conversation

DaveCTurner
Copy link
Contributor

Relates #88201

@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v8.16.0 v9.0.0 labels Oct 1, 2024
@DaveCTurner DaveCTurner mentioned this pull request Oct 1, 2024
Closed
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Oct 1, 2024
@elasticsearchmachine
Copy link
Collaborator

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

DiannaHohensee
DiannaHohensee reviewed Oct 1, 2024
View reviewed changes
...est-http/src/javaRestTest/java/org/elasticsearch/http/IndicesSegmentsRestCancellationIT.java
issueUrl = "https://github.com/elastic/elasticsearch/issues/88201",
value = "org.elasticsearch.http.BlockedSearcherRestCancellationTestCase:DEBUG"
+ ",org.elasticsearch.transport.TransportService:TRACE"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test for comparison? The test failure ticket doesn't appear to mention testIndicesSegmentsRestCancellation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's unclear whether they're really different or not, and given that it's only failing once every few months it seems wise to gather more data here.

...tp/src/javaRestTest/java/org/elasticsearch/http/BlockedSearcherRestCancellationTestCase.java Outdated
try {
searcherBlock.acquire();
} catch (InterruptedException e) {
throw new AssertionError(e);
}
searcherBlock.release();

if (testCaseLogger.isDebugEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is just to ascertain whether to do work involved in preparing the DEBUG log message, right? Do we care about that performance in testing infrastructure? Or is this simply following good coding practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test performance is less important than production performance but not totally unimportant. Probably doesn't matter here too much but a bare logger.debug() with expensive params should just look wrong regardless of context.

...tp/src/javaRestTest/java/org/elasticsearch/http/BlockedSearcherRestCancellationTestCase.java Outdated
@@ -57,6 +61,8 @@
*/
public abstract class BlockedSearcherRestCancellationTestCase extends HttpSmokeTestCase {

private static final Logger testCaseLogger = LogManager.getLogger(BlockedSearcherRestCancellationTestCase.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

[opt] ESTestCase is also a test case. Could say blockedTestCaseLogger, to clarify it's this test case specifically. Or a comment saying you're deviating from the norm because there's already a logger -- I wondered why and went to look.

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's really because logger means Engine#logger in the places we add logging here, and that's pretty noisy.

...tp/src/javaRestTest/java/org/elasticsearch/http/BlockedSearcherRestCancellationTestCase.java
Thread.currentThread().getName(),
searcherBlock.availablePermits()
),
new ElasticsearchException("stack trace")
Copy link
Contributor

@DiannaHohensee DiannaHohensee Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, this throws an exception every time debug() is called? Why would we want that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it just constructs the exception so we have a stack trace to log.

mhl-b
mhl-b approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@mhl-b mhl-b left a comment

Choose a reason for hiding this comment

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

LGTM

Is there anything specific you expect to see from new logging?

@DaveCTurner
Copy link
Contributor Author

I'm wondering if we are calling acquireSearcher for reasons other than executing the indices segments action, meaning that sometimes we consider the operation blocked too soon to cancel it properly. But maybe we'll see something interesting in the sequence of transport messages.

@DaveCTurner DaveCTurner added auto-backport-and-merge auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Oct 2, 2024
@elasticsearchmachine elasticsearchmachine merged commit cd42719 into elastic:main Oct 3, 2024
16 checks passed
@DaveCTurner DaveCTurner deleted the 2024/10/01/IndicesSegmentsRestCancellationIT-more-logging branch October 3, 2024 09:54
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Oct 3, 2024
@DaveCTurner
More verbose logging in IndicesSegmentsRestCancellationIT (elastic#…
70c4f24 
…113844)

Relates elastic#88201
@DaveCTurner DaveCTurner mentioned this pull request Oct 3, 2024
Merged
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 8.x

elasticsearchmachine pushed a commit that referenced this pull request Oct 3, 2024
@DaveCTurner
More verbose logging in IndicesSegmentsRestCancellationIT (#113844) (…
c32f815 
…#114003)

Relates #88201
matthewabbott pushed a commit to matthewabbott/elasticsearch that referenced this pull request Oct 4, 2024
@DaveCTurner @matthewabbott
More verbose logging in IndicesSegmentsRestCancellationIT (elastic#…
067e804 
…113844)

Relates elastic#88201
@ywangd ywangd mentioned this pull request Jan 13, 2025
Merged
@ywangd
Copy link
Member

ywangd commented Jan 13, 2025

💚 All backports created successfully

Status Branch Result
✅ 7.17

Questions ?

Please refer to the Backport tool documentation

ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jan 13, 2025
@DaveCTurner
More verbose logging in IndicesSegmentsRestCancellationIT (elastic#…
7a8ac58 
…113844)

Relates elastic#88201

(cherry picked from commit cd42719)
elasticsearchmachine pushed a commit that referenced this pull request Jan 14, 2025
@ywangd @DaveCTurner
[7.17] More verbose logging in IndicesSegmentsRestCancellationIT (#…
92cb5f1 
…113844) (#120025)

* More verbose logging in `IndicesSegmentsRestCancellationIT` (#113844)

Relates #88201

(cherry picked from commit cd42719)

* Fix logger reference

* Use String instead of Strings

---------

Co-authored-by: David Turner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhl-b mhl-b mhl-b approved these changes

@DiannaHohensee DiannaHohensee Awaiting requested review from DiannaHohensee

Assignees
No one assigned
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. >test Issues or PRs that are addressing/adding tests v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DaveCTurner @elasticsearchmachine @ywangd @DiannaHohensee @mhl-b