Fixing the regression in inner hits aggregation

[jainankitk]

This commit fixing the regression introduced by da5b205.

Description

Reverts #12503 that introduced regression in 2.13.0. Thanks a lot @martijnbolhuis for spotting it and supplying the tests, @dblock for working with the contributor.
Related Issues

Closes #13467

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  - [ ] Public documentation issue/PR created

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.

[jainankitk]

Related to #13486

[reta]

Related to #13486

I am not comfortable with this change as is - we need to add more tests, not just one, to cover all uncovered flows and catch the regressions.

[jainankitk]

I am not comfortable with this change as is - we need to add more tests, not just one, to cover all uncovered flows and catch the regressions.

@dblock @reta - Ideally the original commit should have been 2 separate PRs one for ScriptFields and InnerHits. My suggestion is to not press panic button and revert everything. Instead make more cognitive choice.

to cover all uncovered flows and catch the regressions.

all is very hard in my opinion, but I am open to suggestions

[reta]

all is very hard in my opinion, but I am open to suggestions

hence we have such regressions, I prefer taking more time to understand were we have misses instead of patching in a rush

[github-actions]

❌ Gradle check result for bbdbc08: FAILURE

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 result for 45ee240: FAILURE

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 result for 23c45d5: FAILURE

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?

[jainankitk]

all is very hard in my opinion, but I am open to suggestions

hence we have such regressions, I prefer taking more time to understand were we have misses instead of patching in a rush

As previously mentioned, those 2 changes should have gone as part of separate commits, and makes sense to revert only one of those. But, I am okay if you really prefer to revert both the changes.

[reta]

@dblock @msfroh wdyt folks? I would go with full revert #13486 and reintroducing those in 2.15.0 once we have sufficient test coverage for both.

[msfroh]

@dblock @msfroh wdyt folks? I would go with full revert #13486 and reintroducing those in 2.15.0 once we have sufficient test coverage for both.

Yeah... I think that makes sense. Let's get back to the known-okay behavior -- as far as I know, we don't have any evidence that initializing the FetchSubPhaseProcessor was causing any noticeable pain(*). We should treat this regression as a helpful sign that we didn't have enough test coverage for these kinds of requests. Once we have the test coverage, we can move forward with the change to avoid unnecessary object creation.

(*) Taking a look at the FetchSubPhaseProcessor implementation in InnerHitsPhase, it looks like it only captures references to searchContext and innerHits, so those would be the fields of the anonymous class. Combined with standard object overhead, I'm guessing a useless FetchPhaseSubProcessor probably wastes less than 20 bytes (assuming 32-bit object references).

[dblock]

@msfroh @reta decide in #13486 (comment)?

[jainankitk]

Closing this PR in favor of #13486