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.

Sign up for GitHub

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

Jump to bottom

Remove Booleans use from XContent and ToXContent #28768

Merged
merged 7 commits into from
Mar 9, 2018
Merged

Remove Booleans use from XContent and ToXContent #28768

merged 7 commits into from
Mar 9, 2018

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Feb 21, 2018

This removes the use of the common.Boolean class from two of the XContent
classes, so they can be decoupled from the ES code as much as possible.

Related to #28754, #28504

@dakrone
Remove Booleans use from XContent and ToXContent
3e5b46f 
This removes the use of the `common.Boolean` class from two of the XContent
classes, so they can be decoupled from the ES code as much as possible.

Related to elastic#28754, elastic#28504
@dakrone dakrone requested a review from nik9000 February 21, 2018 15:48
jasontedor
jasontedor requested changes Feb 26, 2018
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I think I miss something here which is why not move Booleans to the core library?

Regarding es.xcontent.strict_duplicate_detection I do wonder if this property is necessary. Is there an open issue to discuss removing this property? Can we consider removing this property in 7.0.0?

@jasontedor jasontedor added the :Core/Infra/Core Core issues without another label label Feb 26, 2018
@jasontedor
Copy link
Member

FYI @elastic/es-core-infra.

@nik9000
Copy link
Member

nik9000 commented Feb 26, 2018

I think I miss something here which is why not move Booleans to the core library?

We talked about making sure the xcontent can go into its own library that doesn't depend on the core library. Moving Booleans to the core library wouldn't let us use it here.

Regarding es.xcontent.strict_duplicate_detection I do wonder if this property is necessary. Is there an open issue to discuss removing this property? Can we consider removing this property in 7.0.0?

Sure! This change has to go back to 6.x so maybe we should talk about doing that as a follow up?

@jasontedor
Copy link
Member

We talked about making sure the xcontent can go into its own library that doesn't depend on the core library. Moving Booleans to the core library wouldn't let us use it here.

I think I want to reconsider this now that the consequences are more in my face. There is something wrong if we are going to start duplicating such basic code.

@dakrone
Copy link
Member Author

dakrone commented Mar 4, 2018

I think I want to reconsider this now that the consequences are more in my
face. There is something wrong if we are going to start duplicating such basic
code.

Okay, with our original constraints we wanted to have XContent be a library in
the libs directory that depended on nothing else. With what we need for this,
we would have an xcontent library where the Booleans class was inside the
xcontent library. ES then would depend on xcontent and be able to use
Booleans (it will still require duplicating two methods from Strings that
Booleans uses so that Strings didn't have to move).

There are a few options:

  • Duplicate the code XContent needs into somewhere (what this currently does)
  • Move Booleans with the XContent code, so that ES and XContent still has
    access to the helpers there
  • Move Booleans into a different library which ES and XContent can both depend
    on (when we discussed this the conclusion was that inter-dependencies between
    projects in libs/ was unwanted)

Did I miss any options? What do we think is best here?

@mattweber
Copy link
Contributor

I hope you publish this new standalone xcontent lib. I would love to use this outside of elasticsearch!

@dakrone
Copy link
Member Author

dakrone commented Mar 7, 2018

/cc @elastic/es-core-infra

@dakrone
Copy link
Member Author

dakrone commented Mar 8, 2018

@jasontedor I made the changes we discussed about keeping it as duplication, but in a helper

jasontedor
jasontedor approved these changes Mar 9, 2018
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left one more comment, no need for another round.

server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java Outdated
* default value if null or the empty string is used. Any other input
* results in an {@link IllegalArgumentException} being thrown.
*/
static boolean parseBoolean(String value, Boolean defaultValue) {
Copy link
Member

Choose a reason for hiding this comment

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

Would you move this to a package-visible class named Booleans? There is no confusion with the other Booleans because of package visibility. It does not make sense on this class, it does not make sense on the other class, and the name will help for ease of finding in the future if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Certainly, I'll do that.

@dakrone dakrone merged commit 97b513e into elastic:master Mar 9, 2018
dakrone added a commit that referenced this pull request Mar 9, 2018
@dakrone
Remove Booleans use from XContent and ToXContent (#28768)
b081967 
* Remove Booleans use from XContent and ToXContent

This removes the use of the `common.Boolean` class from two of the XContent
classes, so they can be decoupled from the ES code as much as possible.

Related to #28754, #28504
@dakrone
Copy link
Member Author

dakrone commented Mar 9, 2018

Thanks everyone

sebasjm pushed a commit to sebasjm/elasticsearch that referenced this pull request Mar 10, 2018
@dakrone
Remove Booleans use from XContent and ToXContent (elastic#28768)
c39b6d3 
* Remove Booleans use from XContent and ToXContent

This removes the use of the `common.Boolean` class from two of the XContent
classes, so they can be decoupled from the ES code as much as possible.

Related to elastic#28754, elastic#28504
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 12, 2018
@jasontedor
Merge branch 'master' into unknown-or-invalid-settings-updates
98a1b6b 
* master: (28 commits)
  Maybe die before failing engine (elastic#28973)
  Remove special handling for _all in nodes info
  Remove Booleans use from XContent and ToXContent (elastic#28768)
  Update Gradle Testing Docs (elastic#28970)
  Make primary-replica resync failures less lenient (elastic#28534)
  Remove temporary file 10_basic.yml~
  Use different pipeline id in test. (pipelines do not get removed between tests extending from ESIntegTestCase)
  Use fixture to test the repository-gcs plugin (elastic#28788)
  Use String.join() to describe a list of tasks (elastic#28941)
  Fixed incorrect test try-catch statement
  Plugins: Consolidate plugin and module loading code (elastic#28815)
  percolator: Take `matchAllDocs` and `verified` of the sub result into account when analyzing a function_score query.
  Build: Remove rest tests on archive distribution projects (elastic#28952)
  Remove FastStringReader in favor of vanilla StringReader (elastic#28944)
  Remove FastCharArrayReader and FastCharArrayWriter (elastic#28951)
  Continue registering pipelines after one pipeline parse failure. (elastic#28752)
  Build: Fix ability to ignore when no tests are run (elastic#28930)
  [rest-api-spec] update doc link for /_rank_eval
  Switch XContentBuilder from BytesStreamOutput to ByteArrayOutputStream (elastic#28945)
  Factor UnknownNamedObjectException into its own class (elastic#28931)
  ...
@dakrone dakrone deleted the xc-remove-booleans branch April 19, 2018 14:46
@danielmitterdorfer danielmitterdorfer mentioned this pull request Dec 14, 2018
Merged
@colings86 colings86 removed the v7.0.0 label Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

@jasontedor jasontedor jasontedor approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >non-issue v6.3.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dakrone @jasontedor @nik9000 @mattweber @colings86