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.

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

Use KeyedLock in IndexFieldDataService #6855

Merged
merged 2 commits into from Jul 16, 2014

Conversation

Projects
None yet
4 participants
@s1monw
Copy link
Contributor

s1monw commented Jul 14, 2014

Today we synchronize when updating the IndexFieldDataService
datastructures. This might unnecessarily block progress if multiple
request need different fielddata instance for different fields.

This commit also fixes clear calls to actually consistently clear
the caches in the case of an exception.

public static <T extends Throwable> T useOrSupress(T first, T second) {
if (first == null) {
return second;
} else {

This comment has been minimized.

Copy link
@nik9000

nik9000 Jul 14, 2014

Contributor

Else right after a return makes me sad. I'm not sure I have a good reason to think that way, but I do.

@s1monw s1monw added review labels Jul 15, 2014

@kimchy

View changes

src/main/java/org/elasticsearch/index/fielddata/IndexFieldDataService.java Outdated
@@ -162,36 +168,62 @@ public void setIndexService(IndexService indexService) {
}

public void clear() {
synchronized (loadedFieldData) {
for (IndexFieldData<?> fieldData : loadedFieldData.values()) {
List<Throwable> exceptions = new ArrayList<>(0);

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 15, 2014

Member

do we want to obtain a global lock here as well?

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Author Contributor

I an not sure if it buys us anything though? it will be consistent afterall IMO as it is?

@kimchy

View changes

src/main/java/org/elasticsearch/ExceptionsHelper.java Outdated
*/
public static <T extends Throwable> void maybeThrowRuntimeAndSuppress(List<T> exceptions) {
if (exceptions.isEmpty() == false) {
RuntimeException rt = new RuntimeException();

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 15, 2014

Member

I am not sure about starting a new RuntimeException, we loose the type of the exception... . Maybe use the first one, and surpass the others?

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Author Contributor

well that doesn't work since the clear calls dont' throw exceptions

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Jul 15, 2014

left a comment about obtaining a global lock on clear, other than that, LGTM

@s1monw

This comment has been minimized.

Copy link
Contributor Author

s1monw commented Jul 15, 2014

@kimchy pushed a new commit

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Jul 15, 2014

LGTM

s1monw added some commits Jul 14, 2014

[FIELDDATA] Use KeyedLock in IndexFieldDataService
Today we synchronize when updating the IndexFieldDataService
datastructures. This might unnecessarily block progress if multiple
request need different fielddata instance for different fields.

This commit also fixes clear calls to actually consistently clear
the caches in the case of an exception.

Closes #6855

@s1monw s1monw merged commit 90ea461 into elastic:master Jul 16, 2014

@s1monw s1monw removed the review label Jul 16, 2014

@s1monw s1monw deleted the s1monw:use_keyed_lock_in_fd branch Jul 16, 2014

s1monw added a commit that referenced this pull request Jul 16, 2014

[FIELDDATA] Use KeyedLock in IndexFieldDataService
Today we synchronize when updating the IndexFieldDataService
datastructures. This might unnecessarily block progress if multiple
request need different fielddata instance for different fields.

This commit also fixes clear calls to actually consistently clear
the caches in the case of an exception.

Closes #6855

@s1monw s1monw removed the review label Jul 16, 2014

@clintongormley clintongormley changed the title [FIELDDATA] Use KeyedLock in IndexFieldDataService Internal: Use KeyedLock in IndexFieldDataService Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Use KeyedLock in IndexFieldDataService Use KeyedLock in IndexFieldDataService Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.