Skip to content

Suppress stacktrace of InterruptedException in CommonCacheNotifier#11715

Merged
abhishekagarwal87 merged 3 commits intoapache:masterfrom
kfaraz:suppress_cachemap_exception
Sep 16, 2021
Merged

Suppress stacktrace of InterruptedException in CommonCacheNotifier#11715
abhishekagarwal87 merged 3 commits intoapache:masterfrom
kfaraz:suppress_cachemap_exception

Conversation

@kfaraz
Copy link
Contributor

@kfaraz kfaraz commented Sep 16, 2021

Addresses #10709

Description

When CommonCachedNotifier is being stopped while the thread is waiting on updateQueue.take(),
an InterruptedException is thrown. The stack trace from this exception gives the wrong idea that something went wrong with the shutdown.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@abhishekagarwal87
Copy link
Contributor

how does the InterruptedException affect graceful shutdown?

LOG.debug(callerName + ":Received responses for cache update notifications.");
}
catch (InterruptedException e) {
LOG.noStackTrace()
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be emitted as an alert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could just log an INFO here instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to me.

@kfaraz
Copy link
Contributor Author

kfaraz commented Sep 16, 2021

The stack trace from the InterruptedException might give the wrong notion that something went wrong with the shutdown.

By graceful shutdown, I meant avoiding the above confusion. Let me fix this description 🙂

@abhishekagarwal87
Copy link
Contributor

The stack trace from the InterruptedException might give the wrong notion that something went wrong with the shutdown.

By graceful shutdown, I meant avoiding the above confusion. Let me fix this description 🙂

Thanks 🙂

@abhishekagarwal87
Copy link
Contributor

Ignoring code coverage check and merging since it is just a logging change.

@abhishekagarwal87 abhishekagarwal87 merged commit 757720f into apache:master Sep 16, 2021
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
@kfaraz kfaraz deleted the suppress_cachemap_exception branch August 1, 2023 04:52
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.

2 participants