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

fix(api): dynamically sets log level for timers pre-created by timer-ng #10133

Closed
wants to merge 1 commit into from

Conversation

gruceo
Copy link
Contributor

@gruceo gruceo commented Jan 18, 2023

If the log level was dynamically set, we must also change the log level for all timers pre-created by the timer-ng library.

We're going to use the global _G table and not a shared dictionary due to performance reasons.

@gruceo gruceo requested review from ADD-SP and a team January 18, 2023 14:45
@ADD-SP
Copy link
Contributor

ADD-SP commented Jan 18, 2023

Just a note, we made a workaround within timer-ng, and while it still has some limitations, we're at least going to make this feature (dynamic log_level) work.

In the future, we should rewrite the C code to implement this feature elegantly, and @gruceo will create internal tickets to track this thiing.

@locao
Copy link
Contributor

locao commented Jan 18, 2023

@gruceo can you check the failed tests, please?

@RobSerafini RobSerafini added this to the 3.2 milestone Jan 19, 2023
@dndx
Copy link
Member

dndx commented Jan 20, 2023

@gruceo Could you put the ticket number for improving dynamic log level in a comment? We need to properly address the hacks we introduced inside our codebase through proper C code change and @ADD-SP can help with that.

@dndx
Copy link
Member

dndx commented Jan 30, 2023

I suggest we take this PR out of 3.2.0 as the hack is too specific to the timer library we use and I have a feeling that there is a better and more systematic way to fix these and similar issues. @ADD-SP can help get this moving towards that direction.

@kikito @guanlan any objections? This does not looks too urgent to me.

@RobSerafini RobSerafini removed this from the 3.2 milestone Jan 31, 2023
@gruceo gruceo force-pushed the fix/dynamic-log-levels-timer-ng branch 3 times, most recently from c44c1d8 to a6a3c9f Compare February 1, 2023 22:49
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 1, 2023
If the log level was dynamically set, we must also change the log level
for all timers pre-created by the timer-ng library.

We're going to use the global _G table and not a shared dictionary due
to performance reasons.
@gruceo gruceo force-pushed the fix/dynamic-log-levels-timer-ng branch from a6a3c9f to 3fa52e9 Compare February 1, 2023 22:51
@gruceo
Copy link
Contributor Author

gruceo commented Feb 2, 2023

@dndx thank you for your suggestion, I dropped a comment to KAG-457. Tests are passing now. Note this bug does not happen with native ngx.timer API. The alternative way to make dynamic log levels work with our timer-ng library is a complete rewrite of our dynamic log levels C code, and it may require a patch (see KAG-457 for details). I suggest we ship this PR as-is for now until we complete work on KAG-457 and since we already uploaded to LuaRocks a new version of the timer-ng library with this fix I don't think it makes sense to merge #10193 without this PR. What do you think? (cc @fffonion @ADD-SP)

@gruceo gruceo requested review from fffonion and dndx February 6, 2023 12:32
@gruceo gruceo closed this Feb 8, 2023
@gruceo gruceo deleted the fix/dynamic-log-levels-timer-ng branch February 8, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants