Skip to content

Always restore the ThreadContext for operations delayed due to a block#23349

Merged
jaymode merged 2 commits intoelastic:masterfrom
jaymode:index_shard_oplock_context
Feb 24, 2017
Merged

Always restore the ThreadContext for operations delayed due to a block#23349
jaymode merged 2 commits intoelastic:masterfrom
jaymode:index_shard_oplock_context

Conversation

@jaymode
Copy link
Copy Markdown
Member

@jaymode jaymode commented Feb 24, 2017

The IndexShardOperationsLock has a mechanism to delay operations if there is currently a block on the lock. These delayed operations are executed when the block is released and are executed by a different thread. When the different thread executes the operations, the ThreadContext is that of the thread that was blocking operations. In order to preserve the ThreadContext, we need to store it and wrap the listener when the operation is delayed.

The IndexShardOperationsLock has a mechanism to delay operations if there is currently a block on the lock. These
delayed operations are executed when the block is released and are executed by a different thread. When the different
thread executes the operations, the ThreadContext is that of the thread that was blocking operations. In order to
preserve the ThreadContext, we need to store it and wrap the listener when the operation is delayed.
Copy link
Copy Markdown
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

Lgtm. Good catch

}
assertFalse(future.isDone());
}
future.get(1, TimeUnit.MINUTES).close();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we make this 1 hour? If it times out it's nice to get thread dump

Copy link
Copy Markdown
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

@bleskes
Copy link
Copy Markdown
Contributor

bleskes commented Feb 24, 2017

LGTM again. I thought about the case where an executor isn't specified and decided to go with you in the logic that "no executor means no threading context support". +1 to being stricter here.

@jaymode jaymode merged commit 5490cb5 into elastic:master Feb 24, 2017
jaymode added a commit that referenced this pull request Feb 24, 2017
#23349)

The IndexShardOperationsLock has a mechanism to delay operations if there is currently a block on the lock. These
delayed operations are executed when the block is released and are executed by a different thread. When the different
thread executes the operations, the ThreadContext is that of the thread that was blocking operations. In order to
preserve the ThreadContext, we need to store it and wrap the listener when the operation is delayed.
jaymode added a commit that referenced this pull request Feb 24, 2017
#23349)

The IndexShardOperationsLock has a mechanism to delay operations if there is currently a block on the lock. These
delayed operations are executed when the block is released and are executed by a different thread. When the different
thread executes the operations, the ThreadContext is that of the thread that was blocking operations. In order to
preserve the ThreadContext, we need to store it and wrap the listener when the operation is delayed.
jaymode added a commit that referenced this pull request Feb 24, 2017
#23349)

The IndexShardOperationsLock has a mechanism to delay operations if there is currently a block on the lock. These
delayed operations are executed when the block is released and are executed by a different thread. When the different
thread executes the operations, the ThreadContext is that of the thread that was blocking operations. In order to
preserve the ThreadContext, we need to store it and wrap the listener when the operation is delayed.
@jaymode jaymode deleted the index_shard_oplock_context branch February 24, 2017 13:39
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 25, 2017
* master: (26 commits)
  CLI: Fix prompting for yes/no to handle console returning null (elastic#23320)
  Tests: Fix reproduce line for packagingTest (elastic#23365)
  Build: Remove extra copies of netty license (elastic#23361)
  [TEST] Removes timeout based wait_for_active_shards REST test (elastic#23360)
  [TEST] increase timeout slightly in wait_for_active_shards test to allow for index creation cluster state update to be processed before ensuring the wait times out
  Handle snapshot repository's missing index.latest
  Adding equals/hashCode to MainResponse (elastic#23352)
  Always restore the ThreadContext for operations delayed due to a block (elastic#23349)
  Add support for named xcontent parsers to high level REST client (elastic#23328)
  Add unit tests for ParentToChildAggregator (elastic#23305)
  Fix after last merge with master and apply last comments
  [INGEST] Lazy load the geoip databases.
  disable BWC tests for the highlighters, need a new 5.x build to make it work
  Expose WordDelimiterGraphTokenFilter (elastic#23327)
  Test that buildCredentials returns correct clazz (elastic#23334)
  Add BreakIteratorBoundaryScanner support for FVH (elastic#23248)
  Prioritize listing index-N blobs over index.latest in reading snapshots (elastic#23333)
  Test: Fix hdfs test fixture setup on windows
  delete and index tests can share some part of the code
  Remove createSampleDocument method and use the sync'ed index method
  ...
jaymode added a commit to jaymode/elasticsearch that referenced this pull request Feb 27, 2017
The ThreadedActionListener may not be invoked by the same thread that creates it. When this happens, the ThreadContext
from the thread that created the ThreadedActionListener should be used. This change makes the ThreadedActionListener
always wrap the ActionListener in a ContextPreservingActionListener to prevent accidental loss of the ThreadContext.

Relates elastic#23349
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants