[Extensions] Create writeable implementations for parser#6364
[Extensions] Create writeable implementations for parser#6364ryanbogan merged 4 commits intoopensearch-project:mainfrom
Conversation
Signed-off-by: Frank Lou <mloufra@amazon.com>
Signed-off-by: Frank Lou <mloufra@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #6364 +/- ##
============================================
+ Coverage 70.58% 70.69% +0.11%
- Complexity 58841 59015 +174
============================================
Files 4799 4799
Lines 282421 282721 +300
Branches 40717 40770 +53
============================================
+ Hits 199341 199871 +530
+ Misses 66584 66484 -100
+ Partials 16496 16366 -130
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Gradle Check (Jenkins) Run Completed with:
|
dbwiddis
left a comment
There was a problem hiding this comment.
LGTM with a few suggestions:
- document what happens with a null parser
- either use the
BooleanParseror get rid of it.
| SettingType type, | ||
| String key, | ||
| Object defaultValue, | ||
| Object parser, |
There was a problem hiding this comment.
If the parser is not Writeable (for example someone uses the constructor directly assigning parser) this could be null. Since that happens way back in lines 98-107 it's not obvious here. Recommend you add @Nullable annotation to this parameter to make it clear (and gain the benefit of code analysis tools double-checking the needed null checks).
In addition it's not obvious (I had to look it up) that the parser instanceof Writeable calls effectively do the null checks. It would be helpful to add a comment at the beginning of this method (about line 159) stating that, to aid people maintaining this code in the future.
| @@ -147,33 +162,96 @@ private Setting<?> createSetting( | |||
| ? Setting.boolSetting(key, (boolean) defaultValue, propertyArray) | |||
There was a problem hiding this comment.
We don't use the BooleanParser here. That's okay because boolSetting() uses the key and propertyArray to recreate it.
However we still use the BooleanParser in logic elsewhere in the code, creating dead code that will never execute, for example the writeParser switch/case for boolean (which means it will never be read by the StreamInput constructor either.
So I'd suggest either creating the (Writeable) BooleanParser here so the other code is actually used, or removing the Boolean case from the reading and writing switches later in this code and deleting the entire BooleanParser code.
There was a problem hiding this comment.
Since we are not using the BooleanParser, I think we can delete it.
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Frank Lou <mloufra@amazon.com>
7d0ec6c to
70c5fda
Compare
Gradle Check (Jenkins) Run Completed with:
|
CHANGELOG.md
Outdated
| - Add a setting to control auto release of OpenSearch managed index creation block ([#6277](https://github.com/opensearch-project/OpenSearch/pull/6277)) | ||
| - Fix timeout error when adding a document to an index with extension running ([#6275](https://github.com/opensearch-project/OpenSearch/pull/6275)) | ||
| - Clean up temporary files created during segment merge incase segment merge fails ([#6324](https://github.com/opensearch-project/OpenSearch/pull/6324)) | ||
| - Create writeable implementations for parser([#6364](https://github.com/opensearch-project/OpenSearch/pull/5838)) |
There was a problem hiding this comment.
Let's skip the changelog for this change
There was a problem hiding this comment.
what do you mean on skip? Are we going to merge another PR first? or just remove this changelog?
There was a problem hiding this comment.
Remove this changelog and add skip changelog label
There was a problem hiding this comment.
you mean add another label like ###Skip and move this changelog under skip label?
There was a problem hiding this comment.
You can delete this entry in the changelog and add the skip-changelog label to your PR
Signed-off-by: Frank Lou <mloufra@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
* make parser writeable Signed-off-by: Frank Lou <mloufra@amazon.com> * add PR link to CHANGELOG Signed-off-by: Frank Lou <mloufra@amazon.com> * address commit Signed-off-by: Frank Lou <mloufra@amazon.com> * remove changelog Signed-off-by: Frank Lou <mloufra@amazon.com> --------- Signed-off-by: Frank Lou <mloufra@amazon.com> (cherry picked from commit cadba4d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* make parser writeable * add PR link to CHANGELOG * address commit * remove changelog --------- (cherry picked from commit cadba4d) Signed-off-by: Frank Lou <mloufra@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Frank Lou mloufra@amazon.com
Description
Create writeable implementations for parser that implement both Function<String, T> and Writeable interfaces for all existing setting implementations that use parsers.
Issues Resolved
Fixes opensearch-project/opensearch-sdk-java#349
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.