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

Core: Add java time xcontent serializers #33120

Merged

Conversation

spinscale
Copy link
Contributor

This ensures that the java time class exposed by painless have proper
serialization/string representations.

Closes #31853

This ensures that the java time class exposed by painless have proper
serialization/string representations.

Closes elastic#31853
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I don't know enough to review any of the changes to the DateFormatters file but the other changes look good to me.

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

@spinscale
Copy link
Contributor Author

the formatter changes fix one small issue in printing out the wrong format, but add a new format which contains more granularity than 999 milliseconds, as this is already done this way in our current ingest code and this remains BWC compatible to the current way of doing things.

One minor change introduced with this one is now, that dates that have a value of 0 milliseconds, those milliseconds are now always printed out - before that one could end up with those milliseconds being removed. I think this is fine for us though.

@spinscale spinscale merged commit 48b388c into elastic:master Aug 29, 2018
spinscale added a commit that referenced this pull request Aug 29, 2018
This ensures that the java time class exposed by painless have proper
serialization/string representations.

Closes #31853
spinscale added a commit that referenced this pull request Aug 29, 2018
This ensures that the java time class exposed by painless have proper
serialization/string representations.

Closes #31853
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)
dnhatn added a commit that referenced this pull request Aug 29, 2018
* 6.x:
  Fix forbidden apis on FIPS (#33202)
  HLRC: Add ML Get Records API (#33085)
  [ML] Fix character set finder bug with unencodable charsets (#33234)
  Tests fix - Graph HLRC client overly long line and syncing core’s copy of GraphExploreResponseTests taken from protocol. Related to #33231
  Test fix - Graph HLRC test was missing field name to be excluded from randomisation logic
  Parse PEM Key files leniantly (#33173)
  Core: Add java time xcontent serializers (#33120)
  Consider multi release jars when running third party audit (#33206)
  Update MSI documentation (#31950)
  [DOCS] Fixes command page titles
  HLRC: Move ML protocol classes into client ml package (#33203)
  Painless: Fix Semicolon Regression (#33212)
@courtneycouch
Copy link

@spinscale Looks like this extends beyond Java time on more recent ES versions:

cannot write xcontent for unknown value of type class org.apache.lucene.search.SortField$2

For example when getting _segments on indexes with lucene sort configurations.

@spinscale
Copy link
Contributor Author

spinscale commented Sep 5, 2018

@courtneycouch can you share your index mapping and index sorting configuration and also provide the version you tested again, and if you got a full stack trace? Also feel free to open a new issue if you want as this is surely not fixed with this PR.

@courtneycouch
Copy link

The simplest way to reproduce it is to do the following on 6.4.0

PUT test_index
{
  "settings": {
    "index": {
      "sort": {"field": ["field_1"]}
    }
  },
  "mappings": {
    "doc": {
      "properties": {
        "field_1": {"type": "keyword"}
      }
    }
  }
}

POST test_index/doc/1
{
 "field_1": "foo"
}

GET test_index/_segments

@courtneycouch
Copy link

Just to add, it works when using a numeric field type. I haven't tested other non-numeric types, I just know when using a keyword type it breaks.

@spinscale
Copy link
Contributor Author

thanks for the reproduction! I used a date field and it did not reproduce... I'll take a look an open an issue.

@spinscale
Copy link
Contributor Author

FYI: I opened #33414

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.

None yet

6 participants