You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@heron.apache.org by GitBox <gi...@apache.org> on 2020/03/18 01:51:52 UTC
[GitHub] [incubator-heron] huijunwu opened a new pull request #3492: revert
the stmgr clientmgr
huijunwu opened a new pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492
fix #3477
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] huijunwu merged pull request #3492: revert the
stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
huijunwu merged pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] nwangtw commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
nwangtw commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394135078
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
Hmm. This is strange because shared_ptr<> should be safer and I can't tell what can be wrong in the original code. @dnrusakov should have better idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] dnrusakov commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
dnrusakov commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394136572
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
cc @nwangtw
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] huijunwu commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
huijunwu commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394498044
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
when a new physical plan is issued from tmaster, the stmgr-client-manager swaps the old stmgr out from array and explicitly calls quit() destructor.
btw, this pr just reverts it to v0.20.0, those lines have not been changed till v0.20.0 for 4 years and we did not see any issue during those 4 years. thus believe it is safe
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] nwangtw commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
nwangtw commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394170123
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
> cc @nwangtw
Thanks.
Yeah I think you are right. Shared ptr is reference based hence this crash is quite strange. The only possible way is a bad cast somewhere that cause the ref count messed up.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] huijunwu commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
huijunwu commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394498044
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
when a new physical plan is issued from tmaster, the stmgr-client-manager swaps the old stmgr out from array and explicitly calls quit() destructor.
btw, this pr just reverts it to v0.20.0, those lines have not been changed till v0.20.1 for 4 years and we did not see any issue during those 4 years. thus believe it is safe
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] huijunwu commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
huijunwu commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394498044
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
when a new physical plan is issued from tmaster, the stmgr-client-manager swaps the old stmgr out from array and explicitly calls quit() destructor.
btw, this pr just reverts it to v0.20.0, those lines have not been changed till v0.20.0 for 4 years and we did not see any issue during those 4 years. believe it is safe
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] dnrusakov commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
dnrusakov commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394136265
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
@huijunw
What you have basically done is allocating some memory but never cleaning it up.
It may lead to memory leaks.
You don't observe that problem any more because now you never call a destructor for `StMgrClientMgr` (previously `shared_ptr` called the destructor of `StMgrClientMgr` as soon as the object is out of scope or the destructor of enclosing object is called).
To fix the original issue you need to run stream manager under Valgrind, then reproduce the crash locally, then take a look at the Valgrind log to see what is the precise stack trace.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] huijunwu commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
huijunwu commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394498044
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
when a new physical plan is issued from tmaster, the stmgr-client-manager swaps the old stmgr out from array and explicitly calls quit() destructor.
btw, this pr just reverts it to v0.20.0, those lines have not been changed till v0.20.0 for 4 years and we did not see any issue during those 4 years
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-heron] dnrusakov commented on a change in pull request
#3492: revert the stmgr clientmgr
Posted by GitBox <gi...@apache.org>.
dnrusakov commented on a change in pull request #3492: revert the stmgr clientmgr
URL: https://github.com/apache/incubator-heron/pull/3492#discussion_r394136265
##########
File path: heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp
##########
@@ -115,7 +115,7 @@ bool StMgrClientMgr::DidAnnounceBackPressure() {
return stream_manager_->DidAnnounceBackPressure();
}
-shared_ptr<StMgrClient> StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
+StMgrClient* StMgrClientMgr::CreateClient(const sp_string& _other_stmgr_id,
Review comment:
@huijunw
What you have basically done is allocating some memory but never cleaning it up.
It may lead to memory leaks.
You don't observe that problem any more because now you never call a destructor for `StMgrClientMgr` (previously `shared_ptr` called the destructor of `StMgrClientMgr` as soon as the object is out of scope or the destrucor of enclosing object is called).
To fix the original issue you need to run stream manager under Valgrind, then reproduce the crash locally, then take a look at the Valgrind log to see what is the precise stack trace.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services