You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/02/13 08:20:15 UTC

[GitHub] [incubator-doris] vagetablechicken opened a new issue #2893: Condition destroy error shouldn't be a fatal error

vagetablechicken opened a new issue #2893: Condition destroy error shouldn't be a fatal error
URL: https://github.com/apache/incubator-doris/issues/2893
 
 
   When we stop one be,  it always makes a fatal error:
   
   `F0213 12:33:53.604131 117681 utils.cpp:1124] fail to destroy cond. err=Device or resource busy`
   
   https://github.com/apache/incubator-doris/blob/fd492e3b6fd729e617536842ba4092911f8afae8/be/src/olap/utils.cpp#L133-L139
   
   We all know that EBUSY means destroy the object referenced by cond while it is referenced by another thread.
   
   It's a common fault in multi-threads, so we shouldn't make it fatal after one try.
   How about make it fatal after several failure attempts? As follows.
   
   ```
   #define PTHREAD_COND_DESTROY_WITH_LOG(condptr) \
       do {\
           int cond_ret = 0;\
           int try_time = 0;\
           while (0 != (cond_ret = pthread_cond_destroy(condptr))) {\
               if (try_time++ < 20) sleep(1); \
               else LOG(FATAL) << "fail to destroy cond. err=" << strerror(cond_ret); \
           }\
       } while (0)
   ```
   
   My test result:
   It will wait 10~15s when the be is idle.
   The cost is a slightly longer time. Or any good method?

----------------------------------------------------------------
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

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] chaoyli commented on issue #2893: Condition destroy error shouldn't be a fatal error

Posted by GitBox <gi...@apache.org>.

chaoyli commented on issue #2893: Condition destroy error shouldn't be a fatal error
URL: https://github.com/apache/incubator-doris/issues/2893#issuecomment-591370257
 
 
   Thread in this place is not elegant and will be refactored will imported ThreadPool framework.
   https://github.com/apache/incubator-doris/pull/2915
   If you have interest, we can discuss to do it.

----------------------------------------------------------------
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

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] vagetablechicken commented on issue #2893: Condition destroy error shouldn't be a fatal error

Posted by GitBox <gi...@apache.org>.

vagetablechicken commented on issue #2893: Condition destroy error shouldn't be a fatal error
URL: https://github.com/apache/incubator-doris/issues/2893#issuecomment-591880810
 
 
   > Thread in this place is not elegant and will be refactored will imported ThreadPool framework.
   > #2915
   > If you have interest, we can discuss to do it.
   
   That's nice. Actually, I'm meant to create a issue about thread pool, which using KUDU as a reference model.
   If TaskWorkerPool was replaced by KUDU::ThreadPool, it can be shutdown. And the variable number of threads is an excellent design.

----------------------------------------------------------------
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

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] vagetablechicken edited a comment on issue #2893: Condition destroy error shouldn't be a fatal error

Posted by GitBox <gi...@apache.org>.

vagetablechicken edited a comment on issue #2893: Condition destroy error shouldn't be a fatal error
URL: https://github.com/apache/incubator-doris/issues/2893#issuecomment-587268035

Yes, this method may not help. But it can avoid coredump when we restart be. Receiving too much coredump alert sms is too painful.
😂
OK, let's consider the more perfect method.

Stack trace is
```
Check failure stack trace: ***
@ 0x22f3e5d google::LogMessage::Fail()
@ 0x22f5ce4 google::LogMessage::SendToLog()
@ 0x22f3984 google::LogMessage::Flush()
@ 0x22f6719 google::LogMessageFatal::~LogMessageFatal()
@ 0xe9f09f doris::Condition::~Condition()
@ 0x13a2d4d doris::TaskWorkerPool::~TaskWorkerPool()
@ 0x13a2f71 doris::TaskWorkerPool::~TaskWorkerPool()
@ 0x13a0b9b doris::AgentServer::~AgentServer()
```
The thread created in TaskWorkerPool will be detached.
https://github.com/apache/incubator-doris/blob/625411bd280270e1aa026a03632f3f7f6140653d/be/src/agent/task_worker_pool.cpp#L261

And because of that, when the Condition is destroying, the work threads may still running.
If we want to ensure that all references should be completed before the object is released, we only have two choices:

1. make threads joinable, waiting all threads finished in ~TaskWorkerPool() (p.s. it's similar with "wait in PTHREAD_COND_DESTROY_WITH_LOG" method. “wait in PTHREAD_COND_DESTROY_WITH_LOG” has a certain wait time, perhaps better.)
1. interrupt the running threads in ~TaskWorkerPool()

----------------------------------------------------------------
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

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] vagetablechicken commented on issue #2893: Condition destroy error shouldn't be a fatal error

Posted by GitBox <gi...@apache.org>.

vagetablechicken commented on issue #2893: Condition destroy error shouldn't be a fatal error
URL: https://github.com/apache/incubator-doris/issues/2893#issuecomment-587268035

Yes, this method may not help. But it can avoid coredump when we restart be. Receiving too much coredump alert sms is too painful.
😂
OK, let's consider the more perfect method.

1. make threads joinable, waiting all threads finished in ~TaskWorkerPool() (p.s. it's similar with "wait in PTHREAD_COND_DESTROY_WITH_LOG" method.)
1. interrupt the running threads in ~TaskWorkerPool()

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] imay commented on issue #2893: Condition destroy error shouldn't be a fatal error

Posted by GitBox <gi...@apache.org>.

imay commented on issue #2893: Condition destroy error shouldn't be a fatal error
URL: https://github.com/apache/incubator-doris/issues/2893#issuecomment-586974693
 
 
   My concern is that even if this is done, it may not solve the problem. I think we need to ensure that all references should be completed before the object is released. Otherwise, the destruction is successful here, and another thread may refer to the object that has already been destroyed, which may cause crash

----------------------------------------------------------------
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

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] vagetablechicken closed issue #2893: Condition destroy error shouldn't be a fatal error

Posted by GitBox <gi...@apache.org>.

vagetablechicken closed issue #2893: Condition destroy error shouldn't be a fatal error
URL: https://github.com/apache/incubator-doris/issues/2893
 
 
   

----------------------------------------------------------------
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

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org