-
Notifications
You must be signed in to change notification settings - Fork 25.6k
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.
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
Improve ShardLockObtainFailedException message
#134198
Conversation
It's unclear whether the age mentioned here is the age of the lock, or just of the detailed state of the lock. This commit clarifies the message.
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| "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]""", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
It's unclear whether the age mentioned here is the age of the lock, or
just of the detailed state of the lock. This commit clarifies the
message.