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.

Sign up for GitHub

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

Jump to bottom

Fixing the deprecation warning for system indices to be logged once #1537

Closed
wants to merge 2 commits into from
Closed

Fixing the deprecation warning for system indices to be logged once #1537

wants to merge 2 commits into from

Conversation

VachaShah
Copy link
Collaborator

Signed-off-by: Vacha [email protected]

Description

The deprecation warnings for system indices to be logged once instead of spamming the user with multiple logs.

Issues Resolved

Closes #1108

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.

@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 4ca51a0

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 4ca51a0

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 4ca51a0
Log 1048

Reports 1048

@VachaShah VachaShah requested a review from a team as a code owner November 11, 2021 22:20
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 35cec89

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 35cec89

dblock
dblock reviewed Nov 11, 2021
View reviewed changes
server/src/main/java/org/opensearch/cluster/metadata/IndexNameExpressionResolver.java
@@ -83,6 +83,8 @@
public static final String SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY = "_system_index_access_allowed";
public static final Version SYSTEM_INDEX_ENFORCEMENT_VERSION = LegacyESVersion.V_7_10_0;

private boolean isDeprecationWarningAlreadyLogged;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a default value?

server/src/main/java/org/opensearch/cluster/metadata/IndexNameExpressionResolver.java
@@ -364,13 +366,14 @@ private void checkSystemIndexAccess(Context context, Metadata metadata, Set<Inde
.map(i -> i.getIndex().getName())
.sorted() // reliable order for testing
.collect(Collectors.toList());
if (resolvedSystemIndices.isEmpty() == false) {
if (resolvedSystemIndices.isEmpty() == false && !isDeprecationWarningAlreadyLogged) {
Copy link
Member

Choose a reason for hiding this comment

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

This has a potential race condition where two threads may end up logging and then flipping the value, no?

@dblock
Copy link
Member

dblock commented Nov 11, 2021 •
edited
Loading

Can we solve this generically in some way where the deprecation logger can be configured to log only once from a given source? Might need to track those messages.

@VachaShah
Copy link
Collaborator Author

Can we solve this generically in some way where the deprecation logger can be configured to log only once from a given source? Might need to track those messages.

I tried the tracking messages in the IndexNameExpressionResolver since it prints the deprecation warning for different indices, but didn't go ahead with that I was concerned that the size of the collection where the messages are being tracked might grow a lot. As for handling it generically in deprecation logger, the deprecation logger is used at a lot of places in the code with different warnings to print. Some of the messages are quite similar (example). Maybe we can change the messages to make them more generic to making tracking easier and generic. WDYT?

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 35cec89
Log 1053

Reports 1053

@VachaShah
Copy link
Collaborator Author

❌   Gradle Check failure 35cec89 Log 1053

Reports 1053

Gradle check failed with:

Expiring Daemon because JVM heap space is exhausted
Build timed out (after 120 minutes). Marking the build as failed.
Build was aborted

@VachaShah
Copy link
Collaborator Author

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 35cec89
Log 1058

Reports 1058

dblock
dblock requested changes Nov 12, 2021
View reviewed changes
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.

I think we want these deprecation messages to get wrapped into a class that is self-aware in terms of whether it has already been logged or not, then only log once. Otherwise we either have to track every message that gets logged, or end up with endless if and booleans all over the place where we log something.

@VachaShah
Copy link
Collaborator Author

I think we want these deprecation messages to get wrapped into a class that is self-aware in terms of whether it has already been logged or not, then only log once. Otherwise we either have to track every message that gets logged, or end up with endless if and booleans all over the place where we log something.

Definitely agree. I will work on this.

@dblock dblock mentioned this pull request Nov 19, 2021
Closed
@VachaShah VachaShah mentioned this pull request Dec 6, 2021
Merged
5 tasks
@VachaShah
Copy link
Collaborator Author

Raised a new PR for all deprecation messages as discussed above. Closing this in favor of #1660

@VachaShah VachaShah closed this Dec 6, 2021
@CEHENKLE CEHENKLE reopened this Dec 6, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 35cec89

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 35cec89

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 35cec89
Log 1344

Reports 1344

@VachaShah VachaShah closed this Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Deprecation message spamming logs from Security plugin
4 participants
@VachaShah @opensearch-ci-bot @dblock @CEHENKLE