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

Remove MetaDataSerivce and it's semaphores #14159

Merged
merged 3 commits into from Oct 19, 2015

Conversation

Projects
None yet
4 participants
@s1monw
Copy link
Contributor

commented Oct 16, 2015

MetaDataSerivce tried to protect concurrent index creation/deletion
from resulting in inconsistent indices. This was originally added a
long time ago via #1296 which seems to be caused by several problems
that we fixed already in 2.0 or even in late 1.x version. Indices where
recreated without being deleted and shards where deleted while being used
which is now prevented on several levels. We can safely remove the semaphores
since we are already serializing the events on the cluster state threads.
This commit also fixes some expception handling bugs exposed by the added test

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2015

@bleskes @javanna can you guys take a look ?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2015

I also think #11258 would become significantly simpler or even obsolete?

listener.onFailure(t);
}
});
} catch (IndexNotFoundException ex) {

This comment has been minimized.

Copy link
@javanna

javanna Oct 16, 2015

Member

is it correct to only catch IndexNotFound here? could we potentially miss other exceptions and forget to notify the listener?

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 16, 2015

Author Contributor

heh yeah I mean we can catch exception but I really wonder if we should do this on the caller of the method? I mean this can be really critical if we miss anything here?

This comment has been minimized.

Copy link
@javanna

javanna Oct 16, 2015

Member

I am not sure. You mean that you would prefer not to have the catch at all, or to catch Exception instead ? :)

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 16, 2015

Author Contributor

I rethrow the excpetion now since I only added it to get the debug logging there and we handle the exception on the caller side anyway so we should be good here...

This comment has been minimized.

Copy link
@javanna

javanna Oct 16, 2015

Member

sounds good to me

@javanna

This comment has been minimized.

Copy link
Member

commented Oct 16, 2015

left a question, LGTM otherwise. I like how it simplifies things, and indeed it helps a lot with #11258.

@bleskes

View changes

core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java Outdated
synchronized (indexDeleteAction) {
indexDeleteAction.incrementAndGet();
}
client().admin().indices().prepareDelete("test").get(); // from here on all docs with index_version == 0|1 must be gone!!!! only 2 are ok;

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 19, 2015

Member

do we want to assertAcked here?

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 19, 2015

Member

was it the intention to put this under synchronized? o.w. I don't see what the sync buy us - we use an atomic integer anyway. Also - isn't it surprising the test passes? I mean an index operation that was already started before the delete (with old version) can re create the index and inject old docs into it?

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 19, 2015

Author Contributor

all I am asserting in this test is that we never reuse an old shard etc. to your not being so suprised this test failed a lot for different reasons if you have a better idea go ahead.

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 19, 2015

Author Contributor

wait the sync here is essential we want to ensure that all in-flight index operations are done before we increment that's the entire deal here? the relevant part is below when we continue indexing

@bleskes

View changes

core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java Outdated

@Override
public void onFailure(Throwable e) {
ExceptionsHelper.reThrowIfNotNull(e);

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 19, 2015

Member

having this called with a null exception is a problem in it's own. I don't think we want to suppress it?

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 19, 2015

Author Contributor

more bike shedding ahead... I will make the change

@bleskes

This comment has been minimized.

Copy link
Member

commented Oct 19, 2015

Change LGTM. Im sure we'll see some fall out from this and fix those but it's the right way to go. Asked some questions about the test.

s1monw added some commits Oct 16, 2015

Remove MetaDataSerivce and it's semaphores
MetaDataSerivce tried to protect concurrent index creation/deletion
from resulting in inconsistent indices. This was originally added a
long time ago via #1296 which seems to be caused by several problems
that we fixed already in 2.0 or even in late 1.x version. Indices where
recreated without being deleted and shards where deleted while being used
which is now prevented on several levels. We can safely remove the semaphores
since we are already serializing the events on the cluster state threads.
This commit also fixes some expception handling bugs exposed by the added test

@s1monw s1monw force-pushed the s1monw:remove_meta_data_service branch to c5cf5cd Oct 19, 2015

s1monw added a commit that referenced this pull request Oct 19, 2015

Merge pull request #14159 from s1monw/remove_meta_data_service
Remove MetaDataSerivce and it's semaphores

@s1monw s1monw merged commit 7dd35c5 into elastic:master Oct 19, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw deleted the s1monw:remove_meta_data_service branch Oct 19, 2015

s1monw added a commit that referenced this pull request Oct 19, 2015

Merge pull request #14159 from s1monw/remove_meta_data_service
Remove MetaDataSerivce and it's semaphores
@bleskes

This comment has been minimized.

I see - it's about waiting for in flight requests from bellow and it's Ok if we have request with higher version go into the previous index an deleted. Thanks for adding the comment.

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.