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.

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

Added Logger which emits a log based on a condition #4488

Draft
wants to merge 4 commits into
base: 2.1
Choose a base branch
from

Conversation

dlmarion
Copy link
Contributor

Fixes #4409

@EdColeman
Copy link
Contributor

I added a more general comment on #4409, While this may be useful in some cases, I'm not sure it solves the original issue.

Mainly it seems that this would be hard to use when there is unique information in the message that should be passed along. If we need to treat "issue tablet x" and "issue tablet y" as different messages and not as repeat occurrences of the same message, this seems to want to treat them the same.

@dlmarion dlmarion changed the title Added IntervalLogger which emits a log msg once per time interval Added Logger which emits a log based on a condition Apr 23, 2024
@dlmarion
Copy link
Contributor Author

In f679cd3 I modified the Logger to be based on a generic condition and added a time based implementation that uses a configuration property for the time interval. Like @EdColeman mentioned, not sure how useful this is, but I can envision wanting to log things in a server when some condition is true.

@EdColeman
Copy link
Contributor

Would there be any benefit of allowing the property to be dynamic and stored in ZooKeeper? That would also require a watcher to react to updates - and that is likely to make using this class much harder, basically a server context would likely be required to get the instance / zoo reader / zoo watcher.

It would really complicate the code, but would allow things to be adjusted without restarting services.

The other draw-back would be because this is a general property, setting it will change all instances and it may be really hard to know what they are.

One alternative could be to use log4j and isLevelEnabled to toggle between messages and or message frequency. Something like the following using a count, or could be modified to use a time.

   if(LOG.isTraceEnabled()){
      LOG.trace("always log");
    } else if(LOG.isDebugEnabled() && (count++ % 10) == 0){
      LOG.debug("log every 10");
    } else if(LOG.isInfoEnabled() && (count++ % 100) == 0) {
      LOG.info("log every 100");
    }

Then, using named loggers and standard configuration, the level for that class could be dynamically adjusted at runtime on a named logger basis?

@dlmarion
Copy link
Contributor Author

dlmarion commented Apr 24, 2024

The other draw-back would be because this is a general property, setting it will change all instances and it may be really hard to know what they are.

yeah, this is a problem. Currently there is no way to modify the properties of a single server at runtime for debug purposes. I think the standard way to do this is using JMX.

With JShell, manipulating JMX beans becomes easier I think.

@dlmarion dlmarion marked this pull request as draft April 24, 2024 15:53
@dlmarion
Copy link
Contributor Author

I had an issue with the implementation regarding AbstractLogger implementing Serializable. I reworked the implementation and removed the Property. I also added @keith-turner's message deduplication implementation to this.

@dlmarion
Copy link
Contributor Author

These changes were merged into #4558

dlmarion added a commit to dlmarion/accumulo that referenced this pull request May 14, 2024
Was having an issue in PR apache#4488 with an earlier version of the code
complaining about Serializable. This seems to be resolved in this
version of the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2.1.3
In progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants