Skip to content

Improve ShardLockObtainFailedException message #134198

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

Merged
merged 2 commits into from
Sep 5, 2025
Merged

Improve ShardLockObtainFailedException message #134198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51f3f15
Improve `ShardLockObtainFailedException` message
DaveCTurner Sep 5, 2025
7a43bdd
Update docs/changelog/134198.yaml
DaveCTurner Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/134198.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 134198
summary: Improve `ShardLockObtainFailedException` message
area: Store
type: enhancement
issues: []
8 changes: 6 additions & 2 deletions server/src/main/java/org/elasticsearch/env/NodeEnvironment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1009,12 +1009,16 @@ void acquire(long timeoutInMillis, final String details) throws ShardLockObtainF
setDetails(details);
} else {
final Tuple<Long, String> lockDetails = this.lockDetails; // single volatile read
final var stateAge = TimeValue.timeValueNanos(System.nanoTime() - lockDetails.v1());
final var message = format(
"obtaining shard lock for [%s] timed out after [%dms], lock already held for [%s] with age [%dms]",
"""
obtaining shard lock for [%s] timed out after [%dms]; \
this shard lock is still held by a different instance of the shard and has been in state [%s] for [%s/%dms]""",
Copy link
Member

Choose a reason for hiding this comment

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

different instance

I think this should be true. Just want to explicitly confirm it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice yes. If it were the same instance of the shard, it wouldn't be trying to acquire a fresh shard lock.

Technically it might not be an IndexShard instance that holds this lock, it could also be one of the processes that deletes unused shard data, but we don't see that happen here and IMO the phrase instance of the shard is sufficiently loosely defined to include those processes too.

details,
timeoutInMillis,
lockDetails.v2(),
TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - lockDetails.v1())
stateAge,
stateAge.millis()
);
maybeLogThreadDump(shardId, message);
throw new ShardLockObtainFailedException(shardId, message);
Expand Down
20 changes: 12 additions & 8 deletions server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
import static org.hamcrest.Matchers.arrayWithSize;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.matchesPattern;
import static org.hamcrest.Matchers.startsWith;

@LuceneTestCase.SuppressFileSystems("ExtrasFS") // TODO: fix test to allow extras
Expand Down Expand Up @@ -134,25 +135,28 @@ public void testShardLock() throws Exception {

try (var mockLog = MockLog.capture(NodeEnvironment.class); var lock = env.shardLock(new ShardId(index, 0), "1")) {
mockLog.addExpectation(
new MockLog.SeenEventExpectation(
"hot threads logging",
NODE_ENVIRONMENT_LOGGER_NAME,
Level.DEBUG,
"hot threads while failing to obtain shard lock for [foo][0]: obtaining shard lock for [2] timed out after *"
)
new MockLog.SeenEventExpectation("hot threads logging", NODE_ENVIRONMENT_LOGGER_NAME, Level.DEBUG, """
hot threads while failing to obtain shard lock for [foo][0]: obtaining shard lock for [2] timed out after [*ms]; \
this shard lock is still held by a different instance of the shard and has been in state [1] for [*/*ms]*""")
);
mockLog.addExpectation(
new MockLog.UnseenEventExpectation(
"second attempt should be suppressed due to throttling",
NODE_ENVIRONMENT_LOGGER_NAME,
Level.DEBUG,
"hot threads while failing to obtain shard lock for [foo][0]: obtaining shard lock for [3] timed out after *"
"*obtaining shard lock for [3] timed out*"
)
);

assertEquals(new ShardId(index, 0), lock.getShardId());

expectThrows(ShardLockObtainFailedException.class, () -> env.shardLock(new ShardId(index, 0), "2"));
assertThat(
expectThrows(ShardLockObtainFailedException.class, () -> env.shardLock(new ShardId(index, 0), "2")).getMessage(),
matchesPattern("""
\\[foo]\\[0]: obtaining shard lock for \\[2] timed out after \\[0ms]; \
this shard lock is still held by a different instance of the shard \
and has been in state \\[1] for \\[.*/[0-9]+ms]""")
);

for (Path path : env.indexPaths(index)) {
Files.createDirectories(path.resolve("0"));
Expand Down