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.

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

Add simple cache for StoreStats #9709

Merged
merged 1 commit into from
Feb 18, 2015
Merged

Add simple cache for StoreStats #9709

merged 1 commit into from
Feb 18, 2015

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Feb 16, 2015

this commit tries to reduce the filesystem calls to fetch metadata
by using a simple cache on top of the stats call.

Relates to #9683

@kimchy
Copy link
Member

kimchy commented Feb 16, 2015

This looks great!, a few comments:

  • You changed OsService to use the new refresh cache thingy, we should also do it on FsService, NetworkService, and ProcessService.
  • The hasChanged flag, if the dir changes not by creating an output, like in the context of shadow replica, then it won't be tripped to recalculate, right? What should we do in this case? I think that maybe we should not use hasChanged, but still maintain a cache of known files and their length, that are created through createOutput (and use it only in estimateSize) so we reduce the length metadata calls?

@s1monw
Copy link
Contributor Author

s1monw commented Feb 16, 2015

You changed OsService to use the new refresh cache thingy, we should also do it on FsService, NetworkService, and ProcessService.

sure will do

The hasChanged flag, if the dir changes not by creating an output, like in the context of shadow replica, then it won't be tripped to recalculate, right? What should we do in this case? I think that maybe we should not use hasChanged, but still maintain a cache of known files and their length, that are created through createOutput (and use it only in estimateSize) so we reduce the length metadata calls

the more I think about this the more I think we should try a different way. I wonder if we can use the Java 7 WatchService and register something on the data dir to get notifications if something has changed from the OS instead of this crazy caching here?

bleskes
bleskes reviewed Feb 16, 2015
View reviewed changes
src/main/java/org/elasticsearch/common/util/SingleObjectCache.java
if (needsRefresh()) {
if(refreshLock.tryLock()) {
try {
cached = refresh();
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need another needsRefresh check here?

@kimchy
Copy link
Member

kimchy commented Feb 16, 2015

the more I think about this the more I think we should try a different way. I wonder if we can use the Java 7 WatchService and register something on the data dir to get notifications if something has changed from the OS instead of this crazy caching here?

I like this idea!

@bleskes
Copy link
Contributor

bleskes commented Feb 16, 2015

left one minor comment. +1 on listening to file system changes.

@s1monw
Copy link
Contributor Author

s1monw commented Feb 17, 2015

@kimchy @bleskes I moved back and went with the cache only solution for now. I added the new cache to all the other places and fixed some tests. can you review?

bleskes
bleskes reviewed Feb 17, 2015
View reviewed changes
src/main/java/org/elasticsearch/index/store/Store.java
@@ -114,6 +119,11 @@ public Store(ShardId shardId, @IndexSettings Settings indexSettings, DirectorySe
this.directory = new StoreDirectory(directoryService.newFromDistributor(distributor), Loggers.getLogger("index.store.deletes", indexSettings, shardId));
this.shardLock = shardLock;
this.onClose = onClose;
final TimeValue refreshInterval = indexSettings.getAsTime(INDEX_STORE_STATS_REFRESH_INTERVAL, TimeValue.timeValueSeconds(10));
this.statsCache = new StoreStatsCache(refreshInterval);
statsCache.getOrRefresh();
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to do this on the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind - I see now we always rely on a valid cached object (because of the non blocking nature). In this case I wonder if we should force passing it to the constructor?

@bleskes
Copy link
Contributor

bleskes commented Feb 17, 2015

Looks much cleaner. I left some comments w.r.t the intialization logic of the cache, where the current semantics require the cached object to be non-null. Maybe we should lock in that case (instead of tryLock)

@s1monw
Copy link
Contributor Author

s1monw commented Feb 18, 2015

@bleskes pushed a new commit - this requires a non-null obj in the ctor.

@bleskes
Copy link
Contributor

bleskes commented Feb 18, 2015

+1

@s1monw
[STORE] Add simple cache for StoreStats
85c611a 
this commit tries to reduce the filesystem calls to fetch metadata
by using a simple cache on top of the stats call.

Relates to elastic#9683
@s1monw s1monw merged commit 85c611a into elastic:master Feb 18, 2015
@bleskes bleskes mentioned this pull request Mar 2, 2015
Closed
bleskes pushed a commit to bleskes/elasticsearch that referenced this pull request Mar 3, 2015
@s1monw @bleskes
[STORE] Add simple cache for StoreStats
45d0abc 
this commit tries to reduce the filesystem calls to fetch metadata
by using a simple cache on top of the stats call.

Relates to elastic#9683

Closes elastic#9709
@bleskes bleskes added the v1.4.5 label Mar 3, 2015
@clintongormley clintongormley added the :Distributed/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. label Mar 19, 2015
@clintongormley clintongormley changed the title [STORE] Add simple cache for StoreStats Add simple cache for StoreStats Jun 7, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
@s1monw @bleskes
[STORE] Add simple cache for StoreStats
a7fc58b 
this commit tries to reduce the filesystem calls to fetch metadata
by using a simple cache on top of the stats call.

Relates to elastic#9683

Closes elastic#9709
@clintongormley clintongormley added :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Distributed/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
:Distributed/Engine Anything around managing Lucene and the Translog in an open shard. >enhancement v1.4.5 v1.5.0 v2.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@s1monw @kimchy @bleskes @clintongormley