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: return ok in kafka-logger #2222

Merged
merged 6 commits into from Sep 17, 2020
Merged

Conversation

dotSlashLu
Copy link
Contributor

@dotSlashLu dotSlashLu commented Sep 14, 2020

What this PR does / why we need it:

local ok, err = self.func(batch.entries, self.batch_max_size)
needs ok and err, but func in kafka-logger returns nil if it's ok. This behaviour causes a lot of unnecessary logs though it's actually ok:

2020/09/14 07:29:38 [error] 67351#0: *180881201 [lua] batch-processor.lua:61: Batch Processor[kafka logger] failed to process entries: nil, context: ngx.timer, client: 10.100.2.45, server: 0.0.0.0:443
2020/09/14 07:29:38 [error] 67351#0: *180881201 [lua] batch-processor.lua:68: Batch Processor[kafka logger] exceeded the max_retry_count[1] dropping the entries, context: ngx.timer, client: 10.100.2.45, server: 0.0.0.0:443

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

@moonming moonming added this to the 2.0 milestone Sep 16, 2020
Copy link
Contributor

@ShiningRush ShiningRush left a comment

Choose a reason for hiding this comment

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

This bug should result in an error for each request, then test case 5 should fail, proving that the case does not cover the situation, I guess it should be caused by the short waiting time, please modify the case to cover the bug

The previouse wait time was too short to expose errors, and by setting
timeout and batch_max_size smaller will make errors to log more quickly
if there are any.
@dotSlashLu
Copy link
Contributor Author

dotSlashLu commented Sep 17, 2020

@ShiningRush thanks for your guide. You are right about why the error message was missed before, such a short waiting time will cover all kinds of error messages including kafka timeout and topic missing.

I've increased the waiting time and also the batch_max_size and timeout in kafka-logger setups to expose errors more quickly if there are any. Now the reproduce is stable.

Before the change:

#   Failed test 'TEST 5: access - pattern "[error]" should not match any line in error.log but matches line "2020/09/17
07:59:38 [error] 2386#0: *20 [lua] batch-processor.lua:61: Batch Processor[kafka logger] failed to process entries: nil, context: ngx.timer, client: 127.0.0.1, server: 0.0.0.0:1984" (req 0)
# '
#   at /usr/local/share/perl/5.28.1/Test/Nginx/Socket.pm line 1280.

#   Failed test 'TEST 5: access - pattern "[error]" should not match any line in error.log but matches line "2020/09/17
07:59:38 [error] 2386#0: *20 [lua] batch-processor.lua:68: Batch Processor[kafka logger] exceeded the max_retry_count[1] dropping the entries, context: ngx.timer, client: 127.0.0.1, server: 0.0.0.0:1984" (req 0)
# '
#   at /usr/local/share/perl/5.28.1/Test/Nginx/Socket.pm line 1280.

@moonming
Copy link
Member

@ShiningRush please take a look, thx

@moonming moonming merged commit d050718 into apache:master Sep 17, 2020
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.

None yet

3 participants