[Extensions] Create writeable implementations for parser

[mloufra]

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.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: null ❌
• URL: null
• CommitID: 0a285d2
  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?

[dbwiddis]

Good intuition to pass the min and max values.

Unfortunately we don't save the min and max values anywhere in the Setting object so at the moment this is impossible to do inside WriteableSetting without modifying Setting.

I've left one idea in the comments, there may be other ways to do it as well.

[dbwiddis]

Anything that you want to read in, you need to write out, so yes you'll somehow need to write the min and max value.

The problem is that when we create a setting we don't save the min and max value so you don't have them, except as stored internally in the private final Function<String, T> parser, which is not writeable.

So you are going to have to go into the Setting class and find where the setting is created, like this for double (similar for other number types):

    public static Setting<Double> doubleSetting(
        String key,
        Setting<Double> fallbackSetting,
        double minValue,
        double maxValue,
        Validator<Double> validator,
        Property... properties
    ) {
        return new Setting<>(
            new SimpleKey(key),
            fallbackSetting,
            fallbackSetting::getRaw,
            (s) -> parseDouble(s, minValue, maxValue, key, isFiltered(properties)),
            validator,
            properties
        );
    }

and replace the function (s) -> parseDouble(s, minValue, maxValue, key, isFiltered(properties)) with something that is writeable.

Here's one idea, you might think of a better one:

public class DoubleParser implements Function<String, T>, Writeable { ... } 

The Function interface will require you implement a method T apply(String s) (the equivalent of the (s) -> ... method that returns T ... syntax) where you would use that parseDouble method. You could instantiate the class with a constructor containing the minValue, maxValue, key, and boolean filtered, and create a new DoubleParser in the above code.

The Writeable interface will require the writeTo() method where you can pass those min/max, key, and filtered values.

Then here, instead of writeMinValue and writeMaxValue you would just write the parser, which would embed the min, max, key, and filtered values.

One great benefit of this approach is it keeps the existing Setting API and gives users ideas/examples of other parsing functions they can include, as long as they are Writeable.

[dbwiddis]

Since you haven't implemented the write portion, I don't know when or if you'd get null. But I think for most of these numbers there are MIN_VALUE for a default min, and MAX_VALUE for a default max, that you will always have a value for it and not have to handle nulls.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9595/
• CommitID: 618df85
  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?

[dbwiddis]

If you make DoubleParser an inner class inside Setting, then:

• You won't need to include the generic type <T> here
• You will automatically gain access to the parseDouble() method inside the Setting class so you don't have to recreate it yourself. That will really simplify the rest of your implementation!

[mloufra]

Agree, I will make DoubleParser as an inner class inside Setting

[dbwiddis]

The min and max can probably be primitive double

[dbwiddis]

Fallback is a Setting<T>

[dbwiddis]

I don't think you need this constructor at all

[dbwiddis]

I think trying to implement this outside the Setting class is overcomplicating things. Don't worry about the fallbacks for now. Just focus on replacing the Function<String, T> parser argument ((s) -> parseDouble(s, minValue, maxValue, key, isFiltered(properties))) with an object.

[dbwiddis]

this isn't really improving on the existing implementation, you're putting a non-Writeable object here for the parser. The whole goal of what you are doing is to have new Setting<Double>( ... writeableParser argument ... ). That way when you send WriteableSetting over transport you can just do parser.writeTo(out).

[dbwiddis]

Java principle: DRY (Don't Repeat Yourself). Use the existing method in Settings object. If it's private, you can just make DoubleParser an inner class and it will have access to it.

public class Setting<T>  ... {
    public class DoubleParser ... { ... }
}

[dbwiddis]

This apply() is what implements parseDouble, the right hand side of the (s) -> parseDouble(s, minValue, maxValue, key, isFiltered(properties)). The (s) is the parameter.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9636/
• CommitID: 5265192
  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?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9660/
• CommitID: 964bb76
  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?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9772/
• CommitID: 1169cc5
  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?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9775/
• CommitID: 6703aea
  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?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9776/
• CommitID: 9274c46
  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?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.snapshots.DedicatedClusterSnapshotRestoreIT.testIndexDeletionDuringSnapshotCreationInQueue
      1 org.opensearch.indices.replication.SegmentReplicationIT.testStartReplicaAfterPrimaryIndexesDocs

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

[github-actions]

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/11317/
• CommitID: 7b8cdf4
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/11316/
• CommitID: 494ee83

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/11318/
• CommitID: 4872ab3
  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?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/11324/
• CommitID: 9da3193

[mloufra]

Open another PR to keep the PR commit clean. Here is the link.