Avoid logging duplicate deprecation warnings multiple times #1660

VachaShah · 2021-12-06T18:55:21Z

Description

This PR adds logic to avoid logging duplicate deprecation messages. It resolves the issue #1108 and all other deprecation message instances. (Coming from #1537 (review)). There are also test changes since a lot of tests verify the deprecation warnings.

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 · 2021-12-06T18:56:03Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2021-12-06T18:57:55Z

✅ Gradle Wrapper Validation success 8d1d07698203277c19fc559b54245cce4314daa2

opensearch-ci-bot · 2021-12-06T19:05:01Z

✅ Gradle Precommit success 8d1d07698203277c19fc559b54245cce4314daa2

opensearch-ci-bot · 2021-12-06T19:12:23Z

❌ Gradle Check failure 8d1d07698203277c19fc559b54245cce4314daa2
Log 1343

Reports 1343

opensearch-ci-bot · 2021-12-06T20:47:59Z

✅ Gradle Wrapper Validation success 47a5366924836114d948056537e37093e9684dcd

opensearch-ci-bot · 2021-12-06T20:56:19Z

✅ Gradle Precommit success 47a5366924836114d948056537e37093e9684dcd

opensearch-ci-bot · 2021-12-06T21:14:04Z

❌ Gradle Check failure 47a5366924836114d948056537e37093e9684dcd
Log 1346

Reports 1346

dblock

Nice work. I would appreciate another pair of eyes other than myself. Maybe @andrross?

andrross · 2021-12-07T18:03:34Z

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java

+        if (keys.contains(this.keyWithXOpaqueId)) {
+            return true;
+        }
+        keys.add(this.keyWithXOpaqueId);


Is this safe on an unsynchronized HashSet? I'm assuming this method can be accessed concurrently from any number of threads. It's likely fine for the de-duplication logic to be best effort and a duplicate log entry emitted due to a race is not a big deal, but I'm more concerned about undefined behavior now or in future JDK versions if the underlying data structure is modified in the middle of a contains() invocation.

Definitely agree! We can use the SynchronizedSet, the only downside might be performance since the collection will be locked by a single thread at a time. If that is not a concern, we can make this synchronized. WDYT?

I think ConcurrentHashMap will give the concurrency guarantees while also allowing lock-free reads. Something like:

private final Set<String> keys = ConcurrentHashMap.newKeySet(); ... public boolean isAlreadyLogged() { return !keys.add(keyWithXOpaqueId); }

Under the hood it looks like the KeySetView of ConcurrentHashMap is using the same method as putIfAbsent in its add method. That should be lock-free in the case that the key is present, and only incur any synchronization overhead in the case that it is newly adding a key.

Sorry for the delay! That makes sense. I have updated the implementation to use the keySet() from ConcurrentHashMap.

test/framework/src/main/java/org/opensearch/test/rest/OpenSearchRestTestCase.java

VachaShah · 2021-12-08T01:02:09Z

start gradle check

opensearch-ci-bot · 2021-12-08T01:33:26Z

❌ Gradle Check failure 47a5366924836114d948056537e37093e9684dcd
Log 1379

Reports 1379

Signed-off-by: Vacha <vachshah@amazon.com>

opensearch-ci-bot · 2021-12-15T07:29:55Z

❌ Gradle Check failure 931bb72d6bac52526117977513ac38c475fea8bb
Log 1502

Reports 1502

VachaShah · 2021-12-15T07:32:33Z

❌ Gradle Check failure 931bb72 Log 1502

Reports 1502

Execution failed for task ':distribution:bwc:bugfix:buildBwcLinuxTar'.
> Building 1.2.1 didn't generate expected file /var/CITOOL/workflow/OpenSearch_CI/PR_Checks/Gradle_Check/search/distribution/bwc/bugfix/build/bwc/checkout-1.2/distribution/archives/linux-tar/build/distributions/opensearch-min-1.2.1-SNAPSHOT-linux-x64.tar.gz

This should resolve after PR #1731 is merged.

opensearch-ci-bot · 2021-12-15T09:02:38Z

❌ Gradle Check failure 998f97c1eda48c805231bf3e52483f680e3e3e11
Log 1503

Reports 1503

andrross · 2021-12-15T16:37:26Z

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java

+     * Otherwise, a warning can be logged by some test and the upcoming test can be impacted by it.
+     */
+    public static void resetDeprecatedMessageForTests() {
+        keys = new HashSet<>();


This isn't right because you're resetting the field to the non-concurrent hashset implementation. I would actually replace this with keys.clear() since the ConcurrentHashMap is thread safe.

Oh my bad! I missed this, I will change it.

andrross · 2021-12-15T16:37:55Z

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java

+    private static Set<String> keys = ConcurrentHashMap.newKeySet();
+    private String keyWithXOpaqueId;


Both of these can be final (see comment below about a change to the reset method).

andrross · 2021-12-15T16:39:53Z

...analysis-common/src/yamlRestTest/resources/rest-api-spec/test/indices.analyze/10_analyze.yml

@@ -19,10 +19,10 @@
    - skip:
        version: " - 6.2.99"
        reason: deprecated in 6.3
-        features: "warnings"
+        features: "allowed_warnings"


I admit that I don't really understand what these files mean, but why did you change "warnings" to "allowed_warnings"?

allowed_warnings asserts if the warning occurs but does not fail the test if it doesn't. This is in a lot of files and there is no order in which they run so it is very difficult to determine when the first warning will occur given that a lot of tests assert the same duplicate warnings.

Signed-off-by: Vacha Shah <vachshah@amazon.com>

opensearch-ci-bot · 2021-12-15T23:04:26Z

✅ Gradle Check success 2330282
Log 1516

Reports 1516

dblock · 2021-12-16T14:52:28Z

Nice job @VachaShah and thanks for a thorough code review @andrross!

* Avoid logging duplicate deprecation warnings multiple times Signed-off-by: Vacha <vachshah@amazon.com> * Fixes test failures Signed-off-by: Vacha <vachshah@amazon.com> * Adding deprecation logger tests Signed-off-by: Vacha <vachshah@amazon.com> * Using ConcurrentHashMap keySet Signed-off-by: Vacha Shah <vachshah@amazon.com> (cherry picked from commit e66ea2c)

…times (#2315) * Avoid logging duplicate deprecation warnings multiple times (#1660) * Avoid logging duplicate deprecation warnings multiple times Signed-off-by: Vacha <vachshah@amazon.com> * Fixes test failures Signed-off-by: Vacha <vachshah@amazon.com> * Adding deprecation logger tests Signed-off-by: Vacha <vachshah@amazon.com> * Using ConcurrentHashMap keySet Signed-off-by: Vacha Shah <vachshah@amazon.com> (cherry picked from commit e66ea2c) * Fixing failing RestResizeHandlerTests in 1.x Signed-off-by: Vacha Shah <vachshah@amazon.com> Co-authored-by: Vacha <vachshah@amazon.com>

VachaShah requested a review from a team as a code owner December 6, 2021 18:55

VachaShah mentioned this pull request Dec 6, 2021

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

Closed

5 tasks

VachaShah added the backport 1.x label Dec 6, 2021

VachaShah force-pushed the deprecation-logs-once branch from 8d1d076 to 47a5366 Compare December 6, 2021 20:43

dblock approved these changes Dec 6, 2021

View reviewed changes

andrross reviewed Dec 7, 2021

View reviewed changes

VachaShah added backport 1.x and removed backport 1.x labels Dec 8, 2021

VachaShah added 3 commits December 14, 2021 16:56

Avoid logging duplicate deprecation warnings multiple times

20844c9

Signed-off-by: Vacha <vachshah@amazon.com>

Fixes test failures

e7d8637

Signed-off-by: Vacha <vachshah@amazon.com>

Adding deprecation logger tests

38119a4

Signed-off-by: Vacha <vachshah@amazon.com>

VachaShah force-pushed the deprecation-logs-once branch from 47a5366 to 931bb72 Compare December 15, 2021 07:08

VachaShah force-pushed the deprecation-logs-once branch from 931bb72 to 998f97c Compare December 15, 2021 08:44

andrross reviewed Dec 15, 2021

View reviewed changes

Using ConcurrentHashMap keySet

2330282

Signed-off-by: Vacha Shah <vachshah@amazon.com>

VachaShah force-pushed the deprecation-logs-once branch from 998f97c to 2330282 Compare December 15, 2021 22:16

VachaShah requested a review from andrross December 15, 2021 23:05

andrross approved these changes Dec 15, 2021

View reviewed changes

VachaShah merged commit e66ea2c into opensearch-project:main Dec 15, 2021

VachaShah deleted the deprecation-logs-once branch December 15, 2021 23:27

VachaShah added backport 1.x and removed backport 1.x labels Mar 2, 2022

opensearch-trigger-bot bot mentioned this pull request Mar 2, 2022

[Backport 1.x] Avoid logging duplicate deprecation warnings multiple times #2315

Merged

VachaShah mentioned this pull request Mar 4, 2022

[BUG] ./gradlew ':qa:full-cluster-restart:v1.3.0#oldClusterTest' fails #2334

Closed

dblock mentioned this pull request Nov 25, 2024

[BUG] DeprecationLogger - Unbounded memory usage #16702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid logging duplicate deprecation warnings multiple times #1660

Avoid logging duplicate deprecation warnings multiple times #1660

VachaShah commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

dblock left a comment

andrross Dec 7, 2021

VachaShah Dec 8, 2021

andrross Dec 8, 2021

VachaShah Dec 15, 2021

VachaShah commented Dec 8, 2021

opensearch-ci-bot commented Dec 8, 2021

opensearch-ci-bot commented Dec 15, 2021

VachaShah commented Dec 15, 2021

opensearch-ci-bot commented Dec 15, 2021

andrross Dec 15, 2021

VachaShah Dec 15, 2021

andrross Dec 15, 2021

andrross Dec 15, 2021

VachaShah Dec 15, 2021

opensearch-ci-bot commented Dec 15, 2021

dblock commented Dec 16, 2021

		private static Set<String> keys = ConcurrentHashMap.newKeySet();
		private String keyWithXOpaqueId;

Avoid logging duplicate deprecation warnings multiple times #1660

Avoid logging duplicate deprecation warnings multiple times #1660

Conversation

VachaShah commented Dec 6, 2021

Description

Issues Resolved

Check List

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

opensearch-ci-bot commented Dec 6, 2021

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VachaShah commented Dec 8, 2021

opensearch-ci-bot commented Dec 8, 2021

opensearch-ci-bot commented Dec 15, 2021

VachaShah commented Dec 15, 2021

opensearch-ci-bot commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Dec 15, 2021

dblock commented Dec 16, 2021