Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guice resection #10921

Closed
wants to merge 1 commit into from
Closed

Guice resection #10921

wants to merge 1 commit into from

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Oct 25, 2023

Surgically removes malignant Guice code.

o.o.common.inject is a fork of Guice 2.0 from many moons ago. It is used to create type safe instances at runtime w/o the need for Factories. Most of the functionality has been removed prior to forking OpenSearch. This housecleaning commit removes stale code that is either not called or unnecessarily called through the Injector logic path.

relates #5910
relates #8110

Surgically removes malignant Guice code.

o.o.common.inject is a fork of Guice from many moons ago. It is used to
create type safe instances at runtime w/o the need for Factories. Most of
the functionality has been removed prior to forking OpenSearch. This
housecleaning commit removes stale code that is either not called or
unnecessarily called through the Injector logic path.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize nknize added enhancement Enhancement or improvement to existing feature or request pending backport Identifies an issue or PR that still needs to be backported v3.0.0 Issues and PRs related to version 3.0.0 skip-changelog v2.12.0 Issues and PRs related to version 2.12.0 labels Oct 25, 2023
@github-actions
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change 4e2ffd0

Incompatible components

Incompatible components: [https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/performance-analyzer.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented Oct 25, 2023

Ah, Jenkins serving up another healthy bowl.
35465_ldementhon_d42d7885-0b48-4e0c-b3b4-4a048f989c3e

@reta reta added the >breaking Identifies a breaking change. label Oct 26, 2023
@reta
Copy link
Collaborator

reta commented Oct 26, 2023

@nknize nice cleanup, we need changelog for it please.

Also, please notice this change is breaking (I've added label to the pull request): the Binder leaks through Plugin::createGuiceModules, we should acknowledge that.

@nknize
Copy link
Collaborator Author

nknize commented Oct 26, 2023

the Binder leaks through Plugin::createGuiceModules.

hideous... that was removed from Elasticsearch a long long time ago. I forgot that it was only merged into 8.0 and we didn't inherit when we forked 7.10.2.

...we should acknowledge that.

+1. I'm going to acknowledge it a step further by deprecating and removing that trappy method from Plugin altogether (in a followup PR). Bless any plugin that customized this guice injection framework since it's been walking the green mile for years.

@dblock
Copy link
Member

dblock commented Oct 30, 2023

Also, please notice this change is breaking (I've added label to the pull request): the Binder leaks through Plugin::createGuiceModules, we should acknowledge that.

Given conversations such as #5902 (comment), what do you think about introducing an UPGRADING.md that would include both dev and end-user breaking changes? e.g. https://github.com/opensearch-project/opensearch-py/blob/main/UPGRADING.md.

@nknize
Copy link
Collaborator Author

nknize commented Nov 13, 2023

...what do you think about introducing an UPGRADING.md that would include both dev and end-user breaking changes?

No objection so long as it doesn't add "one more thing" to maintainers todo list. The learning curve to contribute is already quite high.

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Dec 13, 2023
@ticheng-aws
Copy link
Contributor

Hi @nknize, the PR is stalled. Do we have any updates?

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Jan 6, 2024
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@stephen-crawford
Copy link
Contributor

@opensearch-project/opensearch-core do we have plans to continue this work? It does not seem like this PR is going to be moved forward by Nick at the moment, so if we want to move forward someone else may need to pick up where this was left off. Otherwise we should close this.

What is the path forward?

@reta
Copy link
Collaborator

reta commented Apr 11, 2024

What is the path forward?

I would give the author the time to finish it (or let it be closed and reopened later). Nick seems to have much better context why the things have been done this way and what the right way is.

@stephen-crawford
Copy link
Contributor

Hi @reta, thanks for following up. Is there a timeframe OpenSearch's maintainers have decided is reasonable for closing stalled PRs? I know there is the auto comment bot, but I think that was turned off; is it 6 months of inactivity like maintainership or just up to discretion?

@reta
Copy link
Collaborator

reta commented Apr 11, 2024

I know there is the auto comment bot, but I think that was turned off;

I think it is on actually

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Apr 14, 2024
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label May 18, 2024
@nknize
Copy link
Collaborator Author

nknize commented Jul 1, 2024

won't be actively working unless needed... too much divergence and code fragmentation at this point. Let me know if anyone is interested in burning this fat in the future.

@nknize nknize closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking Identifies a breaking change. enhancement Enhancement or improvement to existing feature or request pending backport Identifies an issue or PR that still needs to be backported skip-changelog stalled Issues that have stalled v2.12.0 Issues and PRs related to version 2.12.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants