Cleanup interfaces around ExtensionManager #7374

peternied · 2023-05-02T23:11:13Z

Description

Cleanup interfaces around ExtensionManager

ExtensionsManager has a whole lot of functionality that shouldn't be
exposed to external classes, marked much of this package protected for a
cleaner interface. Then updated the Noop version to override all public
functions to remove extranious feature flag checks.

Note; I didn't go so far to remove all getters/setters that were unused
but that could be a good follow up in the future.

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.

ExtensionsManager has a whole lot of functionality that shouldn't be exposed to external classes, marked much of this package protected for a cleaner interface. Then updated the Noop version to override all public functions to remove extranious feature flag checks. Note; I didn't go so far to remove all getters/setters that were unused but that could be a good follow up in the future. Signed-off-by: Peter Nied <petern@amazon.com>

peternied · 2023-05-02T23:14:43Z

Hi @dbwiddis @varuntumbe @owaiskazi19 @ryanbogan @mloufra @andrross - you've made changes to the ExtensionManager in the past and your review would be valuable as you have context around the construction/design choices.

server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java

server/src/main/java/org/opensearch/node/Node.java

github-actions · 2023-05-02T23:21:28Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14762/
CommitID: 5eead5e
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: Peter Nied <petern@amazon.com>

github-actions · 2023-05-02T23:50:17Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14767/
CommitID: ef079d8
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: Peter Nied <petern@amazon.com>

github-actions · 2023-05-03T00:13:30Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14769/
CommitID: d087609
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 · 2023-05-03T03:05:32Z

Gradle Check (Jenkins) Run Completed with:

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

ryanbogan

Looks good to me! I didn't know that Noop implementations could simplify the code so much.

owaiskazi19

Thanks @peternied for letting us know a better alternative to handle feature flags.

CHANGELOG.md

Signed-off-by: Peter Nied <petern@amazon.com>

peternied · 2023-05-03T19:21:50Z

It makes me so happy clean merges don't dismiss approvals on PRs, not sure when this was added by GitHub but its great!

github-actions · 2023-05-03T19:43:28Z

Gradle Check (Jenkins) Run Completed with:

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

peternied · 2023-05-03T20:06:15Z

New flaky test

* What went wrong:
Execution failed for task ':modules:analysis-common:test'.
> Process 'Gradle Test Executor 587' finished with non-zero exit value 1
  This problem might be caused by incorrect test process configuration.
  Please refer to the test execution section in the User Manual at https://docs.gradle.org/8.1.1/userguide/java_testing.html#sec:test_execution

org.gradle.internal.remote.internal.ConnectException: Could not connect to server [304ccce8-64cf-4a10-93d3-4a8ccc2aafc7 port:45325, addresses:[/127.0.0.1]]. Tried addresses: [/127.0.0.1].
	at org.gradle.internal.remote.internal.inet.TcpOutgoingConnector.connect(TcpOutgoingConnector.java:67)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedClient.getConnection(MessageHubBackedClient.java:36)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:103)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:65)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
Caused by: java.net.ConnectException: Connection refused
	at java.****/sun.nio.ch.Net.pollConnect(Native Method)
	at java.****/sun.nio.ch.Net.pollConnectNow(Net.java:672)
	at java.****/sun.nio.ch.SocketChannelImpl.finishTimedConnect(SocketChannelImpl.java:1141)
	at java.****/sun.nio.ch.SocketChannelImpl.blockingConnect(SocketChannelImpl.java:1183)
	at java.****/sun.nio.ch.SocketAdaptor.connect(SocketAdaptor.java:98)
	at org.gradle.internal.remote.internal.inet.TcpOutgoingConnector.tryConnect(TcpOutgoingConnector.java:81)
	at org.gradle.internal.remote.internal.inet.TcpOutgoingConnector.connect(TcpOutgoingConnector.java:54)
	... 5 more

github-actions · 2023-05-03T20:39:45Z

Gradle Check (Jenkins) Run Completed with:

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

codecov-commenter · 2023-05-03T20:46:13Z

Codecov Report

Merging #7374 (d9275d6) into main (b65047f) will increase coverage by 0.17%.
The diff coverage is 60.60%.

📣 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    #7374      +/-   ##
============================================
+ Coverage     70.53%   70.71%   +0.17%     
- Complexity    59551    59605      +54     
============================================
  Files          4876     4876              
  Lines        285817   285740      -77     
  Branches      41162    41156       -6     
============================================
+ Hits         201609   202067     +458     
+ Misses        67632    67078     -554     
- Partials      16576    16595      +19

Impacted Files	Coverage Δ
...a/org/opensearch/extensions/ExtensionsManager.java	`47.26% <18.18%> (+2.01%)`	⬆️
...g/opensearch/extensions/NoopExtensionsManager.java	`66.66% <66.66%> (-33.34%)`	⬇️
server/src/main/java/org/opensearch/node/Node.java	`86.15% <91.66%> (+1.73%)`	⬆️
...in/java/org/opensearch/indices/IndicesService.java	`70.39% <100.00%> (+8.04%)`	⬆️

... and 509 files with indirect coverage changes

Signed-off-by: Peter Nied <petern@amazon.com>

github-actions · 2023-05-03T21:40:53Z

Gradle Check (Jenkins) Run Completed with:

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

peternied · 2023-05-03T21:48:02Z

ERROR: script returned exit code 128

FYI @mch2 I fix the merge conflict on the CHANGELOG.md and then saw that the CI checks were still all passing - assumed this was OK and hit merge. After the change was merged I saw that the CI checks on the new commit hadn't started up yet. I assume because this PR was merged it generated some strange state in the Jenkins workflow.

IMO I don't think anything will go awry because of this change but just in case this would be the place to start looking if something goes wrong.

server/src/main/java/org/opensearch/extensions/ExtensionsManager.java

cwperks · 2023-05-05T15:43:47Z

@peternied This should be backported to 2.x. Can you add the backport label?

cwperks · 2023-05-05T16:01:29Z

@saratvemulapalli @owaiskazi19 @ryanbogan Can you add a backport 2.x label to this PR?

opensearch-trigger-bot · 2023-05-05T16:03:14Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7374-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3687d0106230186afec58dd5e7b0a58c80f6f7d1
# Push it to GitHub
git push --set-upstream origin backport/backport-7374-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7374-to-2.x.

peternied · 2023-05-05T17:43:16Z

I'll manually backport

* Cleanup interfaces around ExtensionManager ExtensionsManager has a whole lot of functionality that shouldn't be exposed to external classes, marked much of this package protected for a cleaner interface. Then updated the Noop version to override all public functions to remove extranious feature flag checks. Note; I didn't go so far to remove all getters/setters that were unused but that could be a good follow up in the future. Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit 3687d01)

* Cleanup interfaces around ExtensionManager ExtensionsManager has a whole lot of functionality that shouldn't be exposed to external classes, marked much of this package protected for a cleaner interface. Then updated the Noop version to override all public functions to remove extranious feature flag checks. Note; I didn't go so far to remove all getters/setters that were unused but that could be a good follow up in the future. Signed-off-by: Peter Nied <petern@amazon.com> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

peternied requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng and VachaShah as code owners May 2, 2023 23:11

peternied commented May 2, 2023

View reviewed changes

server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java Show resolved Hide resolved

owaiskazi19 reviewed May 2, 2023

View reviewed changes

server/src/main/java/org/opensearch/node/Node.java Show resolved Hide resolved

peternied added 2 commits May 2, 2023 23:39

Remove unneeded duplicate constructors

ef079d8

Signed-off-by: Peter Nied <petern@amazon.com>

Update constructor usage on ExtensionManager

d087609

Signed-off-by: Peter Nied <petern@amazon.com>

Add changelog entry

480b8e8

Signed-off-by: Peter Nied <petern@amazon.com>

peternied mentioned this pull request May 3, 2023

[BUG] [Build Break] buildBwcLinuxTar is expecting 2.7.0 artifact but getting 2.7.1 #7396

Open

ryanbogan approved these changes May 3, 2023

View reviewed changes

owaiskazi19 approved these changes May 3, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into clean-extension-manager

d9275d6

Signed-off-by: Peter Nied <petern@amazon.com>

peternied mentioned this pull request May 3, 2023

[AUTOCUT] Gradle Check Failure. #6132

Closed

Merge branch 'main' into clean-extension-manager

386950a

Signed-off-by: Peter Nied <petern@amazon.com>

peternied merged commit 3687d01 into opensearch-project:main May 3, 2023

peternied deleted the clean-extension-manager branch May 3, 2023 21:36

peternied added the skip-changelog label May 3, 2023

cwperks mentioned this pull request May 4, 2023

Use updated method to lookup extension by unique id opensearch-project/security#2741

Merged

3 tasks

cwperks reviewed May 4, 2023

View reviewed changes

server/src/main/java/org/opensearch/extensions/ExtensionsManager.java Show resolved Hide resolved

owaiskazi19 added the backport 2.x Backport to 2.x branch label May 5, 2023

peternied mentioned this pull request May 5, 2023

[Backport 2.x] Cleanup interfaces around ExtensionManager (#7374) #7451

Merged

6 tasks

cwperks mentioned this pull request May 8, 2023

[BUG] Extensions handshake is failing with TLS after change to utilize lookupInitializedExtensionById opensearch-project/security#2747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup interfaces around ExtensionManager #7374

Cleanup interfaces around ExtensionManager #7374

peternied commented May 2, 2023

peternied commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 3, 2023

github-actions bot commented May 3, 2023

ryanbogan left a comment

owaiskazi19 left a comment

peternied commented May 3, 2023

github-actions bot commented May 3, 2023

peternied commented May 3, 2023

github-actions bot commented May 3, 2023

codecov-commenter commented May 3, 2023

github-actions bot commented May 3, 2023

peternied commented May 3, 2023

cwperks commented May 5, 2023

cwperks commented May 5, 2023

opensearch-trigger-bot bot commented May 5, 2023

peternied commented May 5, 2023

Cleanup interfaces around ExtensionManager #7374

Cleanup interfaces around ExtensionManager #7374

Conversation

peternied commented May 2, 2023

Description

Check List

peternied commented May 2, 2023

github-actions bot commented May 2, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented May 2, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented May 3, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented May 3, 2023

Gradle Check (Jenkins) Run Completed with:

ryanbogan left a comment

Choose a reason for hiding this comment

owaiskazi19 left a comment

Choose a reason for hiding this comment

peternied commented May 3, 2023

github-actions bot commented May 3, 2023

Gradle Check (Jenkins) Run Completed with:

peternied commented May 3, 2023

github-actions bot commented May 3, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented May 3, 2023

Codecov Report

github-actions bot commented May 3, 2023

Gradle Check (Jenkins) Run Completed with:

peternied commented May 3, 2023

cwperks commented May 5, 2023

cwperks commented May 5, 2023

opensearch-trigger-bot bot commented May 5, 2023

peternied commented May 5, 2023