Require MediaType in Strings.toString API

[nknize]

Strings.toString is tightly coupled to XContentType.JSON which blocks modularizing foundation classes from the :server module to the opensearch-core library. This change refactors the Strings.toString API to accept a generic MediaType such that the toString implementation detail is encapsulated to the :server module and the abstraction can be teased into the supporting libraries.

relates #5910

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationDuringQueryPhase
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockIsRemovedWhenAnyNodesNotExceedHighWatermark

• URL: https://build.ci.opensearch.org/job/gradle-check/9895/
• CommitID: 9a48c7d
  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.index.IndexServiceTests.testAsyncTranslogTrimTaskOnClosedIndex
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockWhenAllNodesExceededHighWatermark

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

[codecov-commenter]

Codecov Report

Merging #6009 (64cde67) into main (715ff72) will increase coverage by 0.10%.
The diff coverage is 26.98%.

📣 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    #6009      +/-   ##
============================================
+ Coverage     70.75%   70.85%   +0.10%     
- Complexity    58720    58778      +58     
============================================
  Files          4771     4771              
  Lines        280818   280820       +2     
  Branches      40568    40568              
============================================
+ Hits         198704   198987     +283     
+ Misses        65824    65524     -300     
- Partials      16290    16309      +19     

Impacted Files	Coverage Δ
...opensearch/client/slm/SnapshotLifecyclePolicy.java	0.00% <0.00%> (ø)
...ch/client/slm/SnapshotLifecyclePolicyMetadata.java	0.00% <0.00%> (ø)
.../opensearch/client/slm/SnapshotLifecycleStats.java	0.00% <0.00%> (ø)
...rch/client/slm/SnapshotRetentionConfiguration.java	0.00% <0.00%> (ø)
...java/org/opensearch/common/xcontent/MediaType.java	100.00% <ø> (ø)
...a/org/opensearch/common/xcontent/XContentType.java	100.00% <ø> (ø)
...h/script/mustache/MultiSearchTemplateResponse.java	59.01% <0.00%> (ø)
...rg/opensearch/index/rankeval/RankEvalResponse.java	98.57% <0.00%> (+11.42%)	⬆️
...va/org/opensearch/index/rankeval/RankEvalSpec.java	96.49% <0.00%> (-0.88%)	⬇️
...a/org/opensearch/index/rankeval/RatedDocument.java	81.03% <0.00%> (ø)
... and 564 more

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

[reta]

Since this change is going into 2.x, the changelog entry was put in the wrong section. That is also the cause of at least one of the cherry-pick conflicts on backport.

@andrross this change is breaking (#6009 (comment)), we could only backport it with #6013 (or similar change)

[nknize]

Since this change is going into 2.x, the changelog entry was put in the wrong section. That is also the cause of at least one of the cherry-pick conflicts on backport.

@andrross this change is breaking (#6009 (comment)), we could only backport it with #6013 (or similar change)

So I'm concerned about backport paralysis in that we prevent progress out of fear of breaking something that's not widely used (or more correctly, not marked as a forward facing API). So let's look a little closer here.

What's breaking? Data formats / indexes are not breaking. DSL API isn't. Java signatures are, but those are on classes that aren't marked as @opensearch.api so we technically do not support bwc. Strings is also marked @opensearch.internal so we are free to break that as needed.

[reta]

@nknize the MediaType public interface change is breaking

[nknize]

@nknize the MediaType public interface change is breaking

But we don't support a formal MediaType API... (e.g., it's not marked @opensearch.api)

[reta]

@nknize the MediaType public interface change is breaking

But we don't support a formal MediaType API... (e.g., it's not marked @opensearch.api)

That's a argument, I don't know if the MediaType missed the @opensearch.api or intentionally not including it (just raising my concerns here).

[nknize]

I don't know if the MediaType missed the @opensearch.api or intentionally not including it (just raising my concerns here).

Well we inherited a codebase with a majority implementation that is not suited to officially support community BWC beyond the public facing DSL and index encodings. So when I applied the annotation label in :server I leaned into everything as being @opensearch.internal unless it was explicitly marked as a Plugin or provided the DSL implementation. This gives us the freedom to clean our house independently of the opensearch-sdk (which is intended to provide a formal BWC supported API).

[reta]

Thanks a lot @nknize , no concerns from my side anymore!

[nknize]

Since this change is going into 2.x, the changelog entry was put in the wrong section.

@andrross I'm guessing I missed the changlog issue discussion. Is there any reason we didn't adopt other project behaviors w/ the changelog and use the issue number instead of the Pull request? Not only would this enforce (the recommendation of) having an issue for every PR it would eliminate this crazy commit dance of having to put a dummy URL in the changelog and push a commit that updates it right after opening the PR.

Moreover, we could standardize on PR titles to look more official and more easily searched like [OpenSearch-3383] Upgrade OpenSearch 2.0 to Lucene 9.2

[andrross]

@nknize I agree with your position, but the reality is that just the Strings#toString utility is used by over 100 files outside of the OpenSearch repository. We have thus far been very accommodating to not break things used by other repos within opensearch-project across minor versions even if they were not intended/not well suited to be external APIs. This change, if backported, will likely break multiple plugins in the distribution build and I suspect there will be pushback.

[andrross]

I'm guessing I missed the changlog issue discussion.

@nknize The discussion is still ongoing: #1868 It's fair to say nobody is happy with the process currently.

[nknize]

We have thus far been very accommodating to not break things used by other repos within opensearch-project across minor versions

This is paralyzing. There's no 3.0 timetable and we should be able to move leaner as a project, even if that means plugin developers have to more closely monitor core changes and open PRs to refactor method signature changes. Ideally CI would be setup to broadcast build failure notifications that the original author could open the PRs on each of the repositories but I get that's not easily configured. In the meantime, committing core to not change even a single method in Strings is a pretty terrible Open Source experience.

Update: until we settle on a backport strategy (which might be as simple as adding @deprecated for one minor release) I'll keep these refactor PRs to main.

[dblock]

Update: until we settle on a backport strategy (which might be as simple as adding @deprecated for one minor release) I'll keep these refactor PRs to main.

AFAIK that's the strategy we've been following.

On Strings#toString utility being[used by over 100 files outside of the OpenSearch repository it probably is used by another 100 in custom plugins that folks have authored that we don't see. Relying on discipline and annotations is good intentions and our best efforts will only reduce the size of the problem at best.