[Extensions] Create writeable implementations for parser #6364

mloufra · 2023-02-17T23:51:01Z

Signed-off-by: Frank Lou [email protected]

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

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

Signed-off-by: Frank Lou <[email protected]>

github-actions · 2023-02-18T00:19:54Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/11330/
CommitID: c75da86

codecov-commenter · 2023-02-18T00:21:46Z

Codecov Report

Merging #6364 (70c5fda) into main (c772596) will increase coverage by 0.11%.
The diff coverage is 68.53%.

📣 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

Impacted Files	Coverage Δ
...g/opensearch/common/settings/WriteableSetting.java	`71.58% <55.78%> (-19.07%)`	⬇️
...dex/store/remote/file/OnDemandBlockIndexInput.java	`79.13% <61.81%> (+6.12%)`	⬆️
...n/java/org/opensearch/common/settings/Setting.java	`86.34% <75.44%> (-3.90%)`	⬇️
...e/remote/file/OnDemandBlockSnapshotIndexInput.java	`72.34% <100.00%> (+0.91%)`	⬆️
...g/opensearch/index/analysis/CharFilterFactory.java	`0.00% <0.00%> (-100.00%)`	⬇️
...adonly/AddIndexBlockClusterStateUpdateRequest.java	`0.00% <0.00%> (-75.00%)`	⬇️
...readonly/TransportVerifyShardIndexBlockAction.java	`9.75% <0.00%> (-73.18%)`	⬇️
...rch/test/junit/listeners/ReproduceInfoPrinter.java	`9.52% <0.00%> (-68.26%)`	⬇️
...n/admin/indices/readonly/AddIndexBlockRequest.java	`17.85% <0.00%> (-53.58%)`	⬇️
...ch/transport/ReceiveTimeoutTransportException.java	`50.00% <0.00%> (-50.00%)`	⬇️
... and 523 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2023-02-18T00:25:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/11331/
CommitID: 722515e

dbwiddis

LGTM with a few suggestions:

document what happens with a null parser
either use the BooleanParser or get rid of it.

dbwiddis · 2023-02-20T19:55:57Z

server/src/main/java/org/opensearch/common/settings/WriteableSetting.java

@@ -138,6 +152,7 @@ private Setting<?> createSetting(
 SettingType type,
 String key,
 Object defaultValue,
+ Object parser,


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.

dbwiddis · 2023-02-20T20:04:10Z

server/src/main/java/org/opensearch/common/settings/WriteableSetting.java

@@ -147,33 +162,96 @@ private Setting<?> createSetting(
 ? Setting.boolSetting(key, (boolean) defaultValue, propertyArray)


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.

Since we are not using the BooleanParser, I think we can delete it.

github-actions · 2023-02-21T00:31:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11452/
CommitID: 7d0ec6c
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Frank Lou <[email protected]>

github-actions · 2023-02-21T01:24:50Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.snapshots.DedicatedClusterSnapshotRestoreIT.testIndexDeletionDuringSnapshotCreationInQueue

URL: https://build.ci.opensearch.org/job/gradle-check/11456/
CommitID: 70c5fda
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

owaiskazi19 · 2023-02-21T01:58:56Z

CHANGELOG.md

@@ -97,6 +97,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add a setting to control auto release of OpenSearch managed index creation block ([#6277](https:/opensearch-project/OpenSearch/pull/6277))
 - Fix timeout error when adding a document to an index with extension running ([#6275](https:/opensearch-project/OpenSearch/pull/6275))
 - Clean up temporary files created during segment merge incase segment merge fails ([#6324](https:/opensearch-project/OpenSearch/pull/6324))
+- Create writeable implementations for parser([#6364](https:/opensearch-project/OpenSearch/pull/5838))


Let's skip the changelog for this change

what do you mean on skip? Are we going to merge another PR first? or just remove this changelog?

Remove this changelog and add skip changelog label

you mean add another label like ###Skip and move this changelog under skip label?

You can delete this entry in the changelog and add the skip-changelog label to your PR

got it, thanks

dbwiddis

LGTM, great work @mloufra

Signed-off-by: Frank Lou <[email protected]>

github-actions · 2023-02-21T18:39:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/11514/
CommitID: 5c56efd

* make parser writeable Signed-off-by: Frank Lou <[email protected]> * add PR link to CHANGELOG Signed-off-by: Frank Lou <[email protected]> * address commit Signed-off-by: Frank Lou <[email protected]> * remove changelog Signed-off-by: Frank Lou <[email protected]> --------- Signed-off-by: Frank Lou <[email protected]> (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 <[email protected]> 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>

make parser writeable

c75da86

Signed-off-by: Frank Lou <[email protected]>

mloufra mentioned this pull request Feb 17, 2023

[Extensions] Create writeable implementations for parser #5838

Closed

6 tasks

add PR link to CHANGELOG

722515e

Signed-off-by: Frank Lou <[email protected]>

dbwiddis reviewed Feb 20, 2023

View reviewed changes

address commit

70c5fda

Signed-off-by: Frank Lou <[email protected]>

mloufra force-pushed the writeable-parser branch from 7d0ec6c to 70c5fda Compare February 21, 2023 00:55

owaiskazi19 reviewed Feb 21, 2023

View reviewed changes

dbwiddis approved these changes Feb 21, 2023

View reviewed changes

mloufra added the skip-changelog label Feb 21, 2023

remove changelog

5c56efd

Signed-off-by: Frank Lou <[email protected]>

owaiskazi19 approved these changes Feb 21, 2023

View reviewed changes

ryanbogan approved these changes Feb 21, 2023

View reviewed changes

ryanbogan merged commit cadba4d into opensearch-project:main Feb 21, 2023

ryanbogan added the backport 2.x Backport to 2.x branch label Feb 21, 2023

opensearch-trigger-bot bot mentioned this pull request Feb 21, 2023

[Backport 2.x] [Extensions] Create writeable implementations for parser #6421

Merged

mloufra deleted the writeable-parser branch February 21, 2023 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Extensions] Create writeable implementations for parser #6364

[Extensions] Create writeable implementations for parser #6364

mloufra commented Feb 17, 2023

github-actions bot commented Feb 18, 2023

codecov-commenter commented Feb 18, 2023 •

edited

Loading

github-actions bot commented Feb 18, 2023

dbwiddis left a comment

dbwiddis Feb 20, 2023

mloufra Feb 20, 2023

dbwiddis Feb 20, 2023

mloufra Feb 20, 2023

mloufra Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

owaiskazi19 Feb 21, 2023

mloufra Feb 21, 2023

owaiskazi19 Feb 21, 2023

mloufra Feb 21, 2023 •

edited

Loading

ryanbogan Feb 21, 2023

mloufra Feb 21, 2023

dbwiddis left a comment

github-actions bot commented Feb 21, 2023

		@@ -147,33 +162,96 @@ private Setting<?> createSetting(
		? Setting.boolSetting(key, (boolean) defaultValue, propertyArray)

[Extensions] Create writeable implementations for parser #6364

[Extensions] Create writeable implementations for parser #6364

Conversation

mloufra commented Feb 17, 2023

Description

Issues Resolved

Check List

github-actions bot commented Feb 18, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Feb 18, 2023 • edited Loading

Codecov Report

github-actions bot commented Feb 18, 2023

Gradle Check (Jenkins) Run Completed with:

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 21, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloufra Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Feb 18, 2023 •

edited

Loading

mloufra Feb 21, 2023 •

edited

Loading