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

[ML] Partition-wise maximum scores #32748

Merged
merged 4 commits into from
Aug 13, 2018

Conversation

edsavage
Copy link
Contributor

@edsavage edsavage commented Aug 9, 2018

Added infrastructure to push through the 'person name field value' to
the normalizer process. This is required by the normalizer to retrieve
the maximum scores for individual partitions.

The native-code normaliser process will currently simply ignore the new field. Therefore it is safe (and desirable) that this change be pushed prior to the corresponding native code (ml-cpp) changes.

Relates to elastic/ml-cpp#181

Added infrastructure to push through the 'person name field value' to
the normalizer process. This is required by the normalizer to retrieve
the maximum scores for individual partitions.
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

This looks like it should work. I just noticed a couple of minor things.

The reason the PR build failed was nothing to do with this change. To solve that, fetch the latest master branch and merge it into your PR branch. Hopefully then the PR CI will go green.

@@ -74,6 +78,7 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeOptionalString(partitionFieldName);
out.writeOptionalString(partitionFieldValue);
out.writeOptionalString(personFieldName);
out.writeOptionalString(personFieldValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is streaming to another node it needs BWC logic. (I'm not sure why this class is wire streamable, as I would have thought instances of it would always be created, used and destroyed on the same node that the normalizer process was running on, but since it is it's best to do it properly.)

    if (out.getVersion().onOrAfter(Version.V_6_5_0)) {
        out.writeOptionalString(personFieldValue);
    }

@@ -62,6 +65,7 @@ public NormalizerResult(StreamInput in) throws IOException {
partitionFieldName = in.readOptionalString();
partitionFieldValue = in.readOptionalString();
personFieldName = in.readOptionalString();
personFieldValue = in.readOptionalString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is streaming from another node it needs BWC logic. (I'm not sure why this class is wire streamable, as I would have thought instances of it would always be created, used and destroyed on the same node that the normalizer process was running on, but since it is it's best to do it properly.)

    if (in.getVersion().onOrAfter(Version.V_6_5_0)) {
        personFieldValue = in.readOptionalString();
    }

@@ -46,6 +46,9 @@ public String getPersonFieldName() {
return bucketInfluencer.getInfluencerFieldName();
}

@Override
public String getPersonFieldValue() { return null; }
Copy link
Contributor

Choose a reason for hiding this comment

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

The all-on-one-line formatting here is different to all the other methods in the file.

IntelliJ might have mislead you into thinking that all the others look like this as it has an option to squash single statement methods on-screen. There's an option to disable that functionality.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@edsavage edsavage merged commit d147cd7 into elastic:master Aug 13, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 13, 2018
…listeners

* elastic/master: (58 commits)
  [ML] Partition-wise maximum scores (elastic#32748)
  [DOCS] XContentBuilder#bytes method removed, using BytesReference.bytes(docBuilder) (elastic#32771)
  HLRC: migration get assistance API (elastic#32744)
  Add a task to run forbiddenapis using cli (elastic#32076)
  [Kerberos] Add debug log statement for exceptions (elastic#32663)
  Make x-pack core pull transport-nio (elastic#32757)
  Painless: Clean Up Whitelist Names (elastic#32791)
  Cat apis: Fix index creation time to use strict date format (elastic#32510)
  Clear Job#finished_time when it is opened (elastic#32605) (elastic#32755)
  Test: Only sniff host metadata for node_selectors (elastic#32750)
  Update scripted metric docs to use `state` variable (elastic#32695)
  Painless: Clean up PainlessCast (elastic#32754)
  [TEST] Certificate NONE not allowed in FIPS JVM (elastic#32753)
  [ML] Refactor ProcessCtrl into Autodetect and Normalizer builders (elastic#32720)
  Access build tools resources (elastic#32201)
  Tests: Disable rolling upgrade tests with system key on fips JVM (elastic#32775)
  HLRC: Ban LoggingDeprecationHandler (elastic#32756)
  Fix test reproducability in AbstractBuilderTestCase setup (elastic#32403)
  Only require java<version>_home env var if needed
  Tests: Muted ScriptDocValuesDatesTests.testJodaTimeBwc
  ...
edsavage added a commit that referenced this pull request Aug 13, 2018
Added infrastructure to push through the 'person name field value' to
the normalizer process. This is required by the normalizer to retrieve
the maximum scores for individual partitions.
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
@edsavage edsavage deleted the per_partition_max_score_pr branch April 12, 2019 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants