-
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
[bugfix] dyups message loss after worker coredump #1156
base: master
Are you sure you want to change the base?
[bugfix] dyups message loss after worker coredump #1156
Conversation
…s-mesage-loss-bugfix
@FengXingYuXin Could you add some test cases for the scenario ? |
@@ -2125,6 +2148,141 @@ ngx_http_dyups_read_msg_locked(ngx_event_t *ev) | |||
} | |||
|
|||
|
|||
static void ngx_dyups_restore_upstreams_from_hist_queue() |
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.
Does word hist
means historical
?
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.
yes, if it's improper for tengine, i can adjust it.
@@ -745,6 +756,13 @@ ngx_dyups_delete_upstream(ngx_str_t *name, ngx_str_t *rv) | |||
status = NGX_HTTP_INTERNAL_SERVER_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.
should added goto finish
after
rc = ngx_http_dyups_send_msg(name, NULL, NGX_DYUPS_DELETE);
if (rc != NGX_OK) {
ngx_str_set(rv, "alert: delte success but not sync to other process");
ngx_log_error(NGX_LOG_ALERT, ngx_cycle->log, 0, "[dyups] %V", &rv);
status = NGX_HTTP_INTERNAL_SERVER_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.
thanks for your remind, and I have added it.
OK, thanks for your remind, I will supply it later. |
hi all Because #1255 (updated to yzprofile/dyups) has been merged. This pr has conflicts again. @FengXingYuXin , you should file a pr to yzprofile/dyups first, our dev team will review your pr in yzprofile/dyups and then merge it to tengine/dyups. |
周峰 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
2307e19
to
eb0e4f1
Compare
thanks I have create a pr:yzprofile/ngx_http_dyups_module#124 |
1 if your _dyups_upstream_conf is not open and worker coredump, the new worker's dyups messages(from the time of master process's last reload oper) will loss。