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

Don't fsync so often in tests #10516

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mikemccand
Contributor

mikemccand commented Apr 9, 2015

This is a big speedup for some tests, e.g. OldIndexBackwardsCompatibilityTests goes from 159 sec (master) down to 49 sec on my dev box with this change:

In Lucene, when tests need to fsync, we only actually do it rarely. This used to be done in MockDirWrapper.sync, but was recently moved it down to MockFileSystem (DisableFsyncFS).

ES hasn't cutover to MockFS yet, so we are now always doing fsync in master...

I think we can do it rarely() like Lucene? I fixed ESMockDirectoryWrapper to do this ... but maybe instead we can cutover to MockFS?

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 9, 2015

Contributor

I am ok with the reflection hacks for now, lets get tests stable!

Can we open up a followup issue to integrate mockfs? I want to know these hacks will go away eventually.

Contributor

rmuir commented Apr 9, 2015

I am ok with the reflection hacks for now, lets get tests stable!

Can we open up a followup issue to integrate mockfs? I want to know these hacks will go away eventually.

@mikemccand

This comment has been minimized.

Show comment
Hide comment
@mikemccand

mikemccand Apr 9, 2015

Contributor

Thanks @rmuir, yeah I'll open a follow-on issue to add MockFS and then remove this...

Contributor

mikemccand commented Apr 9, 2015

Thanks @rmuir, yeah I'll open a follow-on issue to add MockFS and then remove this...

field = MockDirectoryWrapper.class.getDeclaredField("randomState");
field.setAccessible(true);
superRandomState = (Random) field.get(this);

This comment has been minimized.

@dakrone

dakrone Apr 9, 2015

Member

Just so I understand, why is it necessary to use the MockDirectoryWrapper's Random instead of the Random passed in to the ElasticsearchMockDirectoryWrapper? Does this mean that they are two different instances and might not follow the same seed?

@dakrone

dakrone Apr 9, 2015

Member

Just so I understand, why is it necessary to use the MockDirectoryWrapper's Random instead of the Random passed in to the ElasticsearchMockDirectoryWrapper? Does this mean that they are two different instances and might not follow the same seed?

This comment has been minimized.

@mikemccand

mikemccand Apr 9, 2015

Contributor

why is it necessary to use the MockDirectoryWrapper's Random instead of the Random passed in to the ElasticsearchMockDirectoryWrapper

Hmm good question ... I was just trying to match what MDW did before Lucene switched to MockFS.

But looking at MDW's reasoning / comment in its ctor: it's necessary that we "fork" the incoming Random to our own private Random because the MDW APIs are accessed from multiple threads, and Lucene's test-framework gets (justifiably) angry when it detects that you are sharing a single Random instance across threads...

@mikemccand

mikemccand Apr 9, 2015

Contributor

why is it necessary to use the MockDirectoryWrapper's Random instead of the Random passed in to the ElasticsearchMockDirectoryWrapper

Hmm good question ... I was just trying to match what MDW did before Lucene switched to MockFS.

But looking at MDW's reasoning / comment in its ctor: it's necessary that we "fork" the incoming Random to our own private Random because the MDW APIs are accessed from multiple threads, and Lucene's test-framework gets (justifiably) angry when it detects that you are sharing a single Random instance across threads...

This comment has been minimized.

@mikemccand

mikemccand Apr 9, 2015

Contributor

Lucene's test-framework gets (justifiably) angry when it detects that you are sharing a single Random instance across threads

Hmm but that's not quite right either, because even after forking our private Random instance, it will be used from multiple threads that call MDW's APIs. So now I'm less sure about how test-framework checks usage of Random across threads...

@mikemccand

mikemccand Apr 9, 2015

Contributor

Lucene's test-framework gets (justifiably) angry when it detects that you are sharing a single Random instance across threads

Hmm but that's not quite right either, because even after forking our private Random instance, it will be used from multiple threads that call MDW's APIs. So now I'm less sure about how test-framework checks usage of Random across threads...

This comment has been minimized.

@dakrone

dakrone Apr 9, 2015

Member

Ahh okay, makes sense. Thanks!

@dakrone

dakrone Apr 9, 2015

Member

Ahh okay, makes sense. Thanks!

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