Skip to content

SOLR-14686: fix MDC log clear in SolrCore.close#583

Merged
dsmiley merged 2 commits intoapache:mainfrom
dsmiley:solr-14686-mdcCoreClose
Feb 2, 2022
Merged

SOLR-14686: fix MDC log clear in SolrCore.close#583
dsmiley merged 2 commits intoapache:mainfrom
dsmiley:solr-14686-mdcCoreClose

Conversation

@dsmiley
Copy link
Contributor

@dsmiley dsmiley commented Feb 1, 2022

https://issues.apache.org/jira/browse/SOLR-14686

Fixing my mistake. Ensure MDC is cleared right before this method returns.

@dsmiley dsmiley requested a review from madrob February 1, 2022 04:35
@madrob
Copy link
Contributor

madrob commented Feb 1, 2022

Do we have a test that demonstrated this? Might not fail, but at least something I can look at the log output from?

@madrob
Copy link
Contributor

madrob commented Feb 1, 2022

An alternative idea would be to move the current implementation of close to _close and then the new implementation of public close would be try { _close() } finally { MDCContext.clear() }

@dsmiley
Copy link
Contributor Author

dsmiley commented Feb 1, 2022

Yeah I was thinking try-finally as well but wasn't sure I wanted to make this change over this. I'll do it.

Copy link
Contributor

@madrob madrob left a comment

Choose a reason for hiding this comment

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

One minor note about the code comments, but otherwise looks fine to me.

A potential future improvement would be if we can figure out how to track MDC depth with ObjectReleaseTracker and then our tests would automatically fail if something was unbalanced.

@Override
public void close() {
int count = refCount.decrementAndGet();
if (count > 0) return; // close is called often, and only actually closes if nothing is using it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like the comment here explaining that close is called often and I'll be a little sad about losing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The javadocs on this method are excellent though, so it seemed redundant

@dsmiley dsmiley merged commit 703782d into apache:main Feb 2, 2022
@dsmiley dsmiley deleted the solr-14686-mdcCoreClose branch February 2, 2022 21:22
dsmiley added a commit that referenced this pull request Feb 2, 2022
this is a fix-up of previous commit for this issue
dsmiley added a commit that referenced this pull request Feb 3, 2022
this is a fix-up of previous commit for this issue
limingnihao pushed a commit to limingnihao/solr that referenced this pull request Feb 9, 2022
* main: (132 commits)
  SOLR-15907 Upgrade JWT dependencies mock-oauth2-server and jose4j
  SOLR-15907: Establish jwt-auth module
  SOLR-15907 Apply spotless only on JWT classes (apache#616)
  SOLR-14569: Configuring a shardHandlerFactory on the /select requestHandler results in HTTP 401 when searching on alias in secured Solr.
  SOLR-15984: Ensure all used dependencies are declared
  SOLR-15964: move javadoc to correct method
  SOLR-15964: Transient cores: don't evict open ones (apache#580)
  SOLR-15259: hl.fragAlignRatio now defaults to 0.33 (apache#573)
  Revert "SOLR-15962: surround parser & highlighting: ensure works"
  SOLR-15124: Fix ThreadDumpHandlerTest (apache#589)
  SOLR-12901: UH: handle highlighting no fields Fixes CustomHighlightComponentTest.
  SOLR-15587: Don't use the UrlScheme singleton on the client-side (apache#460)
  SOLR-12901: make hl.method=unified the default (apache#579)
  SOLR-15973: Fix TestSchemaDesignerConfigSetHelper#testDownloadAndZip failure on Windows
  SOLR-15558: Identify zombie processes when stopping
  SOLR-15852 Update smoketester for 9.0 (apache#581)
  SOLR-14686: fix MDC log clear in SolrCore.close (apache#583)
  SOLR-15886: remove showItems (apache#490)
  SOLR-15972 Remove gradle-wrapper.jar (apache#590)
  SOLR-15926: Fix git links and versions in the ref guide (apache#548)
  ...
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