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

HSEARCH-2146 Waiting until ES indexes have become green #1023

Closed
wants to merge 3 commits into from

Conversation

@gunnarmorling
Copy link
Member

commented Mar 7, 2016

The second commit aims at parallelizing index creation. While it works it feels a bit ad-hoc-ish. Let me know what you think. It cuts down test execution time a bit more, but of obviously the effect will be larger if there are more indexes involved.

Note that I'm handling (index creation + mapping creation) as one execution unit, one could think about splitting these two up, too. This wouldn't improve the end-to-end time, but it'd free threads when waiting for index creation to finish. I don't think it's needed right now.

.setParameter( "wait_for_status", "green" )
.setParameter( "timeout", indexManagementWaitTimeout );

Health health = new Health(healthBuilder ) {

This comment has been minimized.

Copy link
@gsmet

gsmet Mar 7, 2016

Member

Minor nitpicking: looks like it doesn't respect the coding styles.

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Mar 7, 2016

Author Member

Indeed; Wondering why CS didn't complain. I'll change it.

return rootFactory;
}

/**
* Does post-initialization of entity managers. Retrieved callables are executed in parallel, then we wait until all

This comment has been minimized.

Copy link
@gsmet

gsmet Mar 7, 2016

Member

Shouldn't it be "index managers"?

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Mar 7, 2016

Author Member

True, I'll change it :)

@gsmet gsmet referenced this pull request Mar 7, 2016
@gsmet

This comment has been minimized.

Copy link
Member

commented Mar 8, 2016

Once the checkstyle issue is fixed, I think we should merge the first commit.

As the second commit goes, probably better to wait for @Sanne .

@gsmet gsmet closed this Mar 8, 2016

@gsmet gsmet reopened this Mar 8, 2016

@gunnarmorling gunnarmorling force-pushed the gunnarmorling:HSEARCH-2146 branch from c611821 to 1523266 Mar 8, 2016

@gunnarmorling

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2016

Addressed @gsmet's comments. Thanks for checking!

@gunnarmorling gunnarmorling force-pushed the gunnarmorling:HSEARCH-2146 branch from 1523266 to 927ca5c Mar 8, 2016

@Sanne Sanne self-assigned this Mar 8, 2016

@@ -100,6 +127,11 @@ private IndexManagementStrategy getIndexManagementStrategy(Properties properties
return strategy != null ? IndexManagementStrategy.valueOf( strategy ) : IndexManagementStrategy.NONE;
}

private String getIndexManagementWaitTimeout(Properties properties) {
String timeout = properties.getProperty( ElasticsearchEnvironment.INDEX_MANAGEMENT_WAIT_TIMEOUT );
return timeout != null ? Integer.valueOf( timeout ) + "ms" : DEFAULT_INDEX_MANAGEMENT_WAIT_TIMEOUT;

This comment has been minimized.

Copy link
@Sanne

Sanne Mar 8, 2016

Member

Do we need better validation, i.e. being a positive number and a reasonable range?

This comment has been minimized.

Copy link
@Sanne

Sanne Mar 8, 2016

Member

And testing for NumberFormatException

This comment has been minimized.

Copy link
@Sanne

Sanne Mar 8, 2016

Member

org.hibernate.search.util.configuration.impl.ConfigurationParseHelper.parseInt(String, int, String)

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Mar 8, 2016

Author Member

Ok, I'll use that helper.

Let's not go overboard with this stuff, though. The prop is documented, and people will get a meaningful exception when giving something else than a number. If someone configures a negative timeout, I think I don't care ;)

This comment has been minimized.

Copy link
@Sanne

Sanne Mar 8, 2016

Member

I might be too strict with user input validation, but in my experience people always have huge amounts of issues with our properties.
I've considered moving to a "typesafe" configuration format like XML but having seen how much pain it introduces when the schemas have to be updated for new features or better formats (i.e. Infinispan) I'm glad we're using properties.. we just need to make sure we're paranoid with validation.

postInitialize.call();
}
catch (Exception e) {
throw new SearchException( "Index manager initialization failed", e );

This comment has been minimized.

Copy link
@Sanne

Sanne Mar 8, 2016

Member

Use the logger to throw exceptions?

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Mar 8, 2016

Author Member

What I head in mind was to Logger-ify everything once we finalize all this work and the dust has settled.

As long as things are in flow, I noticed one moves forth and back with creating these logger methods which seems like a waste of time.

This comment has been minimized.

Copy link
@Sanne

Sanne Mar 8, 2016

Member

Ok that's fine by me.

This comment has been minimized.

Copy link
@gunnarmorling

gunnarmorling Mar 9, 2016

Author Member

I've opened HSEARCH-2168 for log interface usage so we don't forget about it.

JestResult result = clientReference.get().executeRequest( health, false );

if ( !result.isSucceeded() ) {
throw new SearchException( "Index " + actualIndexName + " wasn't created in time; Reason: " + result.getErrorMessage() );

This comment has been minimized.

Copy link
@Sanne

Sanne Mar 8, 2016

Member

Use the logger to throw exceptions?

@gunnarmorling gunnarmorling force-pushed the gunnarmorling:HSEARCH-2146 branch from 927ca5c to 182e4c0 Mar 9, 2016

@gunnarmorling

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2016

@Sanne Reworked some of the things you mentioned and force-pushed.

Note that I've removed the commit which parallelized index creation as I noticed it was causing data races when it comes to indexes shared between different entities. This will need some more consideration.

@Sanne

This comment has been minimized.

Copy link
Member

commented Mar 9, 2016

thanks ! And +1 for the safe approach.

@gunnarmorling

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2016

It's not that it wasn't "safe", it just would drop/create the index multiple times during initialization. Tests were all passing.

@Sanne

This comment has been minimized.

Copy link
Member

commented Mar 9, 2016

merged, except the test as discussed on hipchat.

@Sanne Sanne closed this Mar 9, 2016

@gunnarmorling gunnarmorling deleted the gunnarmorling:HSEARCH-2146 branch Mar 11, 2016

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