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

It is possible to disable NodeAndClusterIdConverter plugin. #4322

Conversation

lukasz-soszynski-eliatra

Description

New system property org.opensearch.common.logging.NodeAndClusterIdConverter.enabled added to disable custom Log4j2 plugin NodeAndClusterIdConverter.

Probably this change needs to be merged to another or additional branch.

Issues Resolved

[List any issues this PR will resolve]

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.

@lukasz-soszynski-eliatra lukasz-soszynski-eliatra requested review from a team and reta as code owners August 29, 2022 16:53
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind explaining why you need this?

Is there precedent where we disable other logging plugins this way elsewhere in the code?

Either way will need tests.

@lukasz-soszynski-eliatra
Copy link
Author

This PR is related to the integration tests for the OpenSearch security plugin. For more details, please see the conversation in another PR: opensearch-project/security#1967 (comment)

@lukasz-soszynski-eliatra lukasz-soszynski-eliatra force-pushed the node-and-cluster-id-converter-configuration branch from d78b562 to 225df5f Compare September 5, 2022 10:23
@lukasz-soszynski-eliatra
Copy link
Author

I have pushed unit tests related to the modified code fragments.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2022

Gradle Check (Jenkins) Run Completed with:

@lukasz-soszynski-eliatra lukasz-soszynski-eliatra force-pushed the node-and-cluster-id-converter-configuration branch from 225df5f to 98de5e7 Compare September 6, 2022 14:38
Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>
@lukasz-soszynski-eliatra lukasz-soszynski-eliatra force-pushed the node-and-cluster-id-converter-configuration branch from 98de5e7 to 7719501 Compare September 6, 2022 14:47
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

Gradle Check (Jenkins) Run Completed with:

@lukasz-soszynski-eliatra
Copy link
Author

I noticed that the build fails with the following error message:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':distribution:bwc:bugfix:buildBwcLinuxTar'.
> Building 2.2.1 didn't generate expected file /var/jenkins/workspace/gradle-check/search/distribution/bwc/bugfix/build/bwc/checkout-2.2/distribution/archives/linux-tar/build/distributions/opensearch-min-2.2.1-SNAPSHOT-linux-x64.tar.gz

I found another error message which seems to be related to the problem:

> Task :distribution:bwc:bugfix:buildBwcLinuxTar FAILED
 [2.2.1] > Task :distribution:archives:buildLinuxTar
 [2.2.1] > Task :distribution:archives:linux-tar:assemble
 [2.2.1] build complete, generating: /var/jenkins/workspace/gradle-check/search/distribution/bwc/bugfix/build/bwc/checkout-2.2/build/[2708.tar.bz](http://2708.tar.bz/)2
 [2.2.1] 
 [2.2.1] BUILD SUCCESSFUL in 3m 38s
 [2.2.1] 169 actionable tasks: 169 executed

Unfortunately, both error messages are not very descriptive. @peternied could you help to figure out what is wrong, please?

@peternied
Copy link
Member

The following warning ApplierService#updateTask][T#1] WARN ClusterApplierService:628 - failed to notify ClusterStateListener occurs when two or more OpenSearch nodes are run in a single JVM. The root cause of the problem is related to the custom Log4j2 plugin NodeAndClusterIdConverter. The plugin stores node and cluster id in the static field

@lukasz-soszynski-eliatra I'm not sure this is the right change, from your description I think the issue is that we cannot run more than one OpenSearch node inside a single JVM without this issue coming up. It seems like the static field should be modified so it can handle this scenario - that would address the root cause.

If you don't see a way to address the root cause, then I'd recommend creating can create an issue on OpenSearch so it can be triaged / reviewed, then we could work around it by overloading the logger with like was done here for JarHell - see where it was discussed #3905

@lukasz-soszynski-eliatra
Copy link
Author

@peternied I created an issue: #4466

@peternied
Copy link
Member

@lukasz-soszynski-eliatra I'm going to close this PR out as I don't think this will be moved forward with. Thanks for the issue!

@peternied peternied closed this Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants