-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: rocketmq logger #5653
feat: rocketmq logger #5653
Conversation
5b1819d
to
d99eb83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start the rocketmq in the https://github.com/apache/apisix/blob/master/ci/pod/docker-compose.yml
0f5e698
to
0a58971
Compare
Pls keep going:-) I'd like to see this feature ready for both community cooperation. |
|
||
repeat_each(1); | ||
no_long_string(); | ||
no_root_location(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We can improve the test in a separate PR:
The newly added test file should use a preprocessor to reduce duplicate --- request
& --- no_error_log
section. You can take a look at:
And please split it into rocketmq-logger.t & rocketmq-logger2.t. Each test file should be smaller than 800L.
return false, "failed to send data to rocketmq topic: " .. err .. | ||
", nameserver_list: " .. core.json.encode(conf.nameserver_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get the nameserver for the current error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I dont understand what you mean. the nameserver is add to err and test case will verify the error log if send fail. such as https://github.com/yuz10/apisix/blob/master/t/plugin/rocketmq-logger.t#L862
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have more nameservers in nameserver_list, one of them is unavailable and this message is sent to the unavailable nameserver, can the log here only print out the unavailable nameserver?
Ok, if this is not convenient to do in this PR, we can optimise it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which nameserver is error can be returned from the lua-resty-rocketmq library, We can create an issue there.
Thank you very much for your outstanding contribution @yuz10 |
What this PR does / why we need it:
feature: add rocketmq-logger plugin
which send log to rocketmq.
Pre-submission checklist: