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

Cleaned up various issues #7787

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@GaelTadh
Copy link
Contributor

GaelTadh commented Sep 18, 2014

Some cleanup to close issues around the indexed scripts API

This closes :
#7560
#7568
#7559
#7647
#7567

@s1monw

View changes

src/main/java/org/elasticsearch/rest/action/script/RestGetIndexedScriptAction.java Outdated

protected String getVersionFieldName() { return "_version"; }

protected String getLangFieldName() { return "lang"; }

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 18, 2014

Contributor

can we just use constants for this?

@s1monw

View changes

src/main/java/org/elasticsearch/search/SearchService.java Outdated
templateContext = new TemplateQueryParser.TemplateContext(innerContext.scriptType(), innerContext.template(), templateContext.params());
parser = null;
try {
parser = XContentFactory.xContent(templateContext.template().getBytes(Charset.defaultCharset())).createParser(templateContext.template().getBytes(Charset.defaultCharset()));

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 18, 2014

Contributor

I think we should pass UTF-8 here instead of default charset?

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 18, 2014

those are good cleanups - can we get them into 1.4 ? I think we should also add a simple BWC test for templates in a different PR

@GaelTadh

This comment has been minimized.

Copy link
Contributor Author

GaelTadh commented Sep 18, 2014

I created issue #7789 to track the BWC test for templates.

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 18, 2014

left some more comments

@s1monw s1monw removed the review label Sep 19, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 19, 2014

@GaelTadh I removed the review tag just FYI

@GaelTadh GaelTadh added the review label Sep 19, 2014

@GaelTadh

This comment has been minimized.

Copy link
Contributor Author

GaelTadh commented Sep 19, 2014

I made the updates.

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 19, 2014

LGTM

@s1monw s1monw removed the review label Sep 19, 2014

Indexed Scripts/Templates : Cleanup
This contains several cleanups to the indexed scripts.
Remove the unused FetchSourceContext from the Get request..
Add lang,_version,_id to the REST GET API.
Removes the routing from GetIndexedScriptRequest since the script index is a single shard that is replicated across all nodes.
Fix backward compatible template file reference
Before 1.3.0 on disk scripts could be referenced by requesting
````
_search/template

{
  "template" : "ondiskscript"
}
````
This was broken in 1.3.0 by requiring
````
{
  "template" :
  {
    "file" : "ondiskscript"
  }
}
````
This commit restores the previous behavior.
Remove support for preference, realtime and refresh
These parameters don't make sense anymore for indexed scripts as we always force the preference to _local and
always refresh after a Put to the indexed scripts index.

Closes #7568
Closes #7559
Closes #7647
Closes #7567

@GaelTadh GaelTadh force-pushed the GaelTadh:indexedscript-cleanup branch to 5c39645 Sep 19, 2014

@GaelTadh GaelTadh closed this Sep 19, 2014

@clintongormley clintongormley changed the title Indexedscript cleanup Indexed Scripts/Templates: Cleaned up various issues Sep 26, 2014

@clintongormley clintongormley added the >bug label Sep 26, 2014

@clintongormley clintongormley changed the title Indexed Scripts/Templates: Cleaned up various issues Cleaned up various issues Jun 7, 2015

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.