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

Call callback on actual mapping processed #6748

Closed
wants to merge 1 commit into from

Conversation

kimchy
Copy link
Member

@kimchy kimchy commented Jul 6, 2014

only callback the registered callback listeners when mapping have actually been processed...

only callback the registered callback listeners when mapping have actually been processed...
closes elastic#6748
if (allTasks == null) {
return;
}
for (Object task : allTasks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should spawn this to a background thread as this is still being run on the cluster state processing thread. Just be on the safe side.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about it, but I think this is the responsibility of the code that adds the listener, spawning (potentially a lot) of threads to callback is expensive, while most times a reply message will just be sent back

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I'm on the fence my self. It's just one (cached) thread we spawn per cluster task (which then executes all of the call backs). Not sure wether you saw this in the profiling as a potential time consumer when it was doing it before. I don't feel too strong about it though.

Copy link
Member Author

Choose a reason for hiding this comment

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

cool, I didn't see it in profiling, but there isn't really a need for it since we just send back the response, I do think we need to properly handle failure if they happen, I will add the failure logic code and push, thanks!

@bleskes
Copy link
Contributor

bleskes commented Jul 6, 2014

Left one minor comment o.w. LGTM

@kimchy
Copy link
Member Author

kimchy commented Jul 6, 2014

@bleskes replied, I think not spawning makes more sense (in retrospect, the previous code did spawn)

@kimchy kimchy closed this in b471aeb Jul 7, 2014
kimchy added a commit that referenced this pull request Jul 7, 2014
only callback the registered callback listeners when mapping have actually been processed...
closes #6748
@kimchy kimchy removed the review label Jul 7, 2014
@kimchy kimchy deleted the callback_mapping_on_processed branch July 7, 2014 07:42
if (task instanceof UpdateTask) {
UpdateTask uTask = (UpdateTask) task;
ClusterStateUpdateResponse response = new ClusterStateUpdateResponse(true);
uTask.listener.onResponse(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

you should really do this in a try / catch block -- we should make sure that all listeners are all in even if one is bogus / throws and exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe have a helper method since the logic below does that though...

@clintongormley clintongormley changed the title Call callback on actual mapping processed Internal: Call callback on actual mapping processed Jul 16, 2014
@clintongormley clintongormley added the :Search/Mapping Index mappings, including merging and defining field types label Jun 7, 2015
@clintongormley clintongormley changed the title Internal: Call callback on actual mapping processed Call callback on actual mapping processed Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Mapping Index mappings, including merging and defining field types v1.3.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants