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

Painless: Add Bindings #33042

Merged
merged 127 commits into from
Aug 29, 2018
Merged

Painless: Add Bindings #33042

merged 127 commits into from
Aug 29, 2018

Conversation

jdconrad
Copy link
Contributor

@jdconrad jdconrad commented Aug 21, 2018

This change allows the specification of a data structure called a binding to store some permanent state within a single script instance. An example binding specified in the whitelist would be as follows:

binding org.elasticsearch.painless.TestBinding {
    (int, int)
    int someCall(int, int)
}

Within the script a user would use this class by simply doing the following:

someCall(1, 2, 3, 4)

where the arguments are split between a call to the constructor the first time this method is called and then the method call for each time the method is called. The constructor has the opportunity to store the arguments passed in as state within the TestBinding class for use within the member method. Each subsequent time the script is run, the parameters are already stored in a local instance so they will not need to be re-calculated.

Each binding must contain exactly one constructor and one method.

@jdconrad jdconrad added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.0.0 v6.5.0 labels Aug 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jdconrad
Copy link
Contributor Author

After some quick discussion with @rjernst I will be changing the formatting of the whitelist to describe bindings shortly with a syntax we think is better than what's being proposed here.

@jdconrad
Copy link
Contributor Author

@rjernst Updated formatting for the whitelist to the following as we spoke about privately.

static {
  int someMethod(int, int, int) bound_to org.package.someClass
}

@jdconrad jdconrad mentioned this pull request Aug 23, 2018
23 tasks
@mayya-sharipova mayya-sharipova mentioned this pull request Aug 24, 2018
9 tasks
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple nits

import java.util.Objects;

/**
* A binding represents a static method call that stores state. Each binding must have exactly
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to distinguish the binding here (whitelist binding) from a binding class? The latter is what most of your description is referring to (except for the first sentence?), but it is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned this up a bit.

// Ensure the final token of the line is '{'.
if (line.endsWith("{") == false) {
throw new IllegalArgumentException(
"invalid class definition: failed to parse class opening bracket [" + line + "]");
Copy link
Member

Choose a reason for hiding this comment

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

Should the message here indicate "static" opening bracket, not class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

addPainlessBinding(targetClass, methodName, returnType, typeParameters);
}

public void addPainlessBinding(Class<?> targetClass, String methodName, Class<?> returnType, List<Class<?>> typeParameters) {
Copy link
Member

Choose a reason for hiding this comment

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

Why two public variants of addPainlessBinding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistency in the API. All of the other add methods allow for things to be added directly as well without using Strings. This allows flexibility for things like testing and such in the future where we don't necessarily want to be forced to use a whitelist.

@jdconrad
Copy link
Contributor Author

@rjernst Thanks for the review. Will commit as soon as CI passes.

@jdconrad jdconrad merged commit b52818e into elastic:master Aug 29, 2018
dnhatn added a commit that referenced this pull request Aug 29, 2018
* master:
  Painless: Add Bindings (#33042)
  Update version after client credentials backport
  Fix forbidden apis on FIPS (#33202)
  Remote 6.x transport BWC Layer for `_shrink` (#33236)
  Test fix - Graph HLRC tests needed another field adding to randomisation exception list
  HLRC: Add ML Get Records API (#33085)
  [ML] Fix character set finder bug with unencodable charsets (#33234)
  TESTS: Fix overly long lines (#33240)
  Test fix - Graph HLRC test was missing field name to be excluded from randomisation logic
  Remove unsupported group_shard_failures parameter (#33208)
  Update BucketUtils#suggestShardSideQueueSize signature (#33210)
  Parse PEM Key files leniantly (#33173)
  INGEST: Add Pipeline Processor (#32473)
  Core: Add java time xcontent serializers (#33120)
  Consider multi release jars when running third party audit (#33206)
  Update MSI documentation (#31950)
  HLRC: create base timed request class (#33216)
  [DOCS] Fixes command page titles
  HLRC: Move ML protocol classes into client ml package (#33203)
  Scroll queries asking for rescore are considered invalid (#32918)
  Painless: Fix Semicolon Regression (#33212)
  ingest: minor - update test to include dissect (#33211)
  Switch remaining LLREST usage to new style Requests (#33171)
  HLREST: add reindex API (#32679)
jdconrad added a commit that referenced this pull request Aug 29, 2018
Add bindings that allow some specialized methods to store permanent state between script executions.
@jdconrad
Copy link
Contributor Author

@mayya-sharipova This is ready to go. Please let me know if you have any issues/need anything else for function scores.

@mayya-sharipova
Copy link
Contributor

@jdconrad Thanks a lot, Jack. Nice to see this change merged, will definitely try to use in function scores.

dnhatn added a commit that referenced this pull request Sep 1, 2018
* 6.x:
  Mute test watcher usage stats output
  [Rollup] Fix FullClusterRestart test
  TEST: Disable soft-deletes in ParentChildTestCase
  TEST: Disable randomized soft-deletes settings
  Integrates soft-deletes into Elasticsearch (#33222)
  drop `index.shard.check_on_startup: fix` (#32279)
  Fix AwaitsFix issue number
  Mute SmokeTestWatcherWithSecurityIT testsi
  [DOCS] Moves ml folder from x-pack/docs to docs (#33248)
  TEST: mute more SmokeTestWatcherWithSecurityIT tests
  [DOCS] Move rollup APIs to docs (#31450)
  [DOCS] Rename X-Pack Commands section (#33005)
  Fixes SecurityIntegTestCase so it always adds at least one alias (#33296)
  TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307)
  MINOR: Remove Dead Code from PathTrie (#33280) (#33306)
  Fix pom for build-tools (#33300)
  Lazy evaluate java9home (#33301)
  SQL: test coverage for JdbcResultSet (#32813)
  Work around to be able to generate eclipse projects (#33295)
  Different handling for security specific errors in the CLI. Fix for #33230 (#33255)
  [ML] Refactor delimited file structure detection (#33233)
  SQL: Support multi-index format as table identifier (#33278)
  Enable forbiddenapis server java9 (#33245)
  [MUTE] SmokeTestWatcherWithSecurityIT flaky tests
  Add region ISO code to GeoIP Ingest plugin (#31669) (#33276)
  Don't be strict for 6.x
  Update serialization versions for custom IndexMetaData backport
  Replace IndexMetaData.Custom with Map-based custom metadata (#32749)
  Painless: Fix Bindings Bug (#33274)
  SQL: prevent duplicate generation for repeated aggs (#33252)
  TEST: Mute testMonitorClusterHealth
  Fix serialization of empty field capabilities response (#33263)
  Fix nested _source retrieval with includes/excludes (#33180)
  [DOCS] TLS file resources are reloadable (#33258)
  Watcher: Ensure TriggerEngine start replaces existing watches (#33157)
  Ignore module-info in jar hell checks (#33011)
  Fix docs build after #33241
  [DOC] Repository GCS ADC not supported (#33238)
  Upgrade to latest Gradle 4.10  (#32801)
  Fix/30904 cluster formation part2 (#32877)
  Move file-based discovery to core (#33241)
  HLRC: add client side RefreshPolicy (#33209)
  [Kerberos] Add unsupported languages for tests (#33253)
  Watcher: Reload properly on remote shard change (#33167)
  Fix classpath security checks for external tests. (#33066)
  [Rollup] Only allow aggregating on multiples of configured interval (#32052)
  Added deprecation warning for rescore in scroll queries (#33070)
  Apply settings filter to get cluster settings API (#33247)
  [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability   (#32743)
  HLRC: create base timed request class (#33216)
  HLRC: Use Optional in validation logic (#33104)
  Painless: Add Bindings (#33042)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants