Skip to content

Stop logging maps in ContextManager#2493

Merged
milleruntime merged 2 commits intoapache:mainfrom
milleruntime:log-cleanup
Feb 14, 2022
Merged

Stop logging maps in ContextManager#2493
milleruntime merged 2 commits intoapache:mainfrom
milleruntime:log-cleanup

Conversation

@milleruntime
Copy link
Copy Markdown
Contributor

No description provided.

@EdColeman
Copy link
Copy Markdown
Contributor

Are these frequent? Does not having a context convey information to the user?

Would something like

log.debug("Managed Contexts: {}", context == null ? "none" : contexts);

be too busy?

@milleruntime
Copy link
Copy Markdown
Contributor Author

Are these frequent? Does not having a context convey information to the user?

They are being logged every minute, so not too crazy but every time 3 messages get printed. I don't think empty context helps users much. This is a deprecated class and was for the old context class loading. But I have never used the feature so maybe someone with more expertise in the class loading can weigh in.

@milleruntime
Copy link
Copy Markdown
Contributor Author

log.debug("Managed Contexts: {}", context == null ? "none" : contexts);

This isn't much different from what we have now. I could combined the two messages so it prints them on one line. I think that would be an improvement but if a user never uses the context class loading, then this is still noise to them.

@EdColeman
Copy link
Copy Markdown
Contributor

I do not know how to classify / balance noise vs. useful information in this case. I can see the viewpoint that if it is useful or important to see the context when you have it configured, the absence could also be informative? - Say you expected it to be set but was not? Or all my others have it set, but why not this one?

It's really hard to go through a log file and find messages that are "missing" - especially if your not sure that you should be looking for them in the first place.

One message seems reasonable as an alternative - but I also don't recall this being and issue - but I am also unsure that if there was a suspected problem, what it would look like and what information would be useful in tracking it down.

@ctubbsii
Copy link
Copy Markdown
Member

Can we just make it so it logs if there are changes?

@milleruntime
Copy link
Copy Markdown
Contributor Author

These log statements are getting printed from AccumuloVFSClassLoader.removeUnusedContexts(contextsInUse) which is called in a thread in startCleanupThread() from the DefaultContextClassLoaderFactory. It seems the whole point of the method is to just to remove unused objects so I don't think we need to be printing every time. From a user standpoint, I think they would only care if something is being removed from the unused map, which is printed in the next loop.

@ctubbsii
Copy link
Copy Markdown
Member

It seems the whole point of the method is to just to remove unused objects so I don't think we need to be printing every time.

Do we log elsewhere when the context is initialized, then?

From a user standpoint, I think they would only care if something is being removed from the unused map, which is printed in the next loop.

If the removals are already being logged, and the initialization is already logged, then I agree it doesn't make sense to continually log the current set here.

@milleruntime
Copy link
Copy Markdown
Contributor Author

Do we log elsewhere when the context is initialized, then?

It is difficult to tell exactly where things get initialized but I found these log statements.

log.debug(
"ClassLoader not created for context {}, creating new one. uris: {}, preDelegation: {}",
cconfig.name, cconfig.uris, cconfig.preDelegation);

log.debug("Returning classloader {} for context {}", loader.getClass().getName(), contextName);

@milleruntime milleruntime changed the title Stop logging empty maps in ContextManager Stop logging maps in ContextManager Feb 14, 2022
@milleruntime milleruntime merged commit 5d6c366 into apache:main Feb 14, 2022
@milleruntime milleruntime deleted the log-cleanup branch February 14, 2022 19:27
@ctubbsii ctubbsii added this to the 2.1.0 milestone Jul 12, 2024
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