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

[Revert of #412] Fixing PA plugin load failure in case of AdmissionController loading … #414

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

gashutos
Copy link
Contributor

…failure
Revert of #412

Is your feature request related to a problem? Please provide an existing Issue # , or describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you are proposing
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

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

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.

…failure

Signed-off-by: Chaitanya Gohel <gashutos@amazon.com>
@khushbr
Copy link
Collaborator

khushbr commented Apr 4, 2023

Hey @gashutos How is this fixing the issue mentioned in #412 ?

The this.getClass().getClassLoader().getParent() will still fail with Caused by: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "getClassLoader") as the plugin will not have the permission to load the parent class loader.

@codecov-commenter
Copy link

Codecov Report

Merging #414 (c4f1c87) into main (e674ccc) will increase coverage by 0.12%.
The diff coverage is 35.71%.

❗ Current head c4f1c87 differs from pull request most recent head 7453bd8. Consider uploading reports for the commit 7453bd8 to get more accurate results

📣 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     #414      +/-   ##
============================================
+ Coverage     72.08%   72.20%   +0.12%     
- Complexity      374      375       +1     
============================================
  Files            44       44              
  Lines          2543     2547       +4     
  Branches        173      173              
============================================
+ Hits           1833     1839       +6     
+ Misses          602      600       -2     
  Partials        108      108              
Impacted Files Coverage Δ
...r/collectors/AdmissionControlMetricsCollector.java 14.66% <35.71%> (+7.62%) ⬆️

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

@gashutos
Copy link
Contributor Author

gashutos commented Apr 6, 2023

Hey @gashutos How is this fixing the issue mentioned in #412 ?

The this.getClass().getClassLoader().getParent() will still fail with Caused by: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "getClassLoader") as the plugin will not have the permission to load the parent class loader.

@khushbr checkout this line, I changed from catching ClassLoaderException to all type of Exceptions here
https://github.com/opensearch-project/performance-analyzer/pull/414/files#diff-5eacd116dc07ed95210f51f4cbaa5afde2a90c6332757aea6a82c80b3cb7d5c9R184
That way we do not want PA to be interrupted by AdmissionControlCollector boot up fail.

@khushbr khushbr merged commit 2a1861b into opensearch-project:main Apr 6, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 6, 2023
…failure (#414)

Signed-off-by: Chaitanya Gohel <gashutos@amazon.com>
(cherry picked from commit 2a1861b)
khushbr pushed a commit that referenced this pull request Apr 6, 2023
…failure (#414) (#418)

Signed-off-by: Chaitanya Gohel <gashutos@amazon.com>
(cherry picked from commit 2a1861b)

Co-authored-by: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants