Bug fix/allow tcp connection to finish #7635
Conversation
ghost
assigned 
arangod/Cluster/ClusterFeature.cpp
Outdated
| @@ -497,6 +497,9 @@ void ClusterFeature::unprepare() { | |||
| AgencyCommManager::MANAGER->stop(); | |||
|
|
|||
| ClusterInfo::cleanup(); | |||
|
|
|||
| // no more tcp requests from now on. | |||
| ApplicationServer::server->beginUnprepare(); | |||
There was a problem hiding this comment.
I think we shouldn't do it like this.
Why is a single feature responsible for unpreparing the entire ApplicationServer?
What if the ClusterFeature is disabled?
| @@ -52,7 +52,8 @@ ApplicationServer::ApplicationServer(std::shared_ptr<ProgramOptions> options, | |||
| : _state(ServerState::UNINITIALIZED), | |||
| _options(options), | |||
| _stopping(false), | |||
| _binaryPath(binaryPath) { | |||
| _binaryPath(binaryPath), | |||
| _unpreparing(false) { | |||
There was a problem hiding this comment.
We shouldn't add a new boolean control variable, as it gets more and more confusing with it.
The ApplicationServer already has a _state variable for the current state it is in. It already has an additional _stopping variable, which we may or may not be redundant. But adding a third state variable does not make it any better from may point of view.
Can't we just use the already existing _state instead of _unpreparing?
| @@ -131,7 +131,9 @@ class ApplicationServer { | |||
| }; | |||
|
|
|||
| static ApplicationServer* server; | |||
|
|
|||
| static bool isUnpreparing() { | |||
| return server != nullptr && server->_unpreparing.load(); | |||
There was a problem hiding this comment.
Can we rather use _state?
| @@ -229,6 +231,11 @@ class ApplicationServer { | |||
| // signal the server to shut down | |||
| void beginShutdown(); | |||
|
|
|||
| // set state to unpreparing phase - no more waiting for external resources from here. | |||
| void beginUnprepare() { | |||
| _unpreparing.exchange(true); | |||
There was a problem hiding this comment.
This is counter-intuitive, as _unpreparing here does not mean the server is in the UNPREPARED state.
| @@ -195,7 +195,7 @@ void StatisticsFeature::start() { | |||
| } | |||
| } | |||
|
|
|||
| void StatisticsFeature::unprepare() { | |||
| void StatisticsFeature::stop() { | |||
There was a problem hiding this comment.
this change was already submitted in another (by now already merged) PR of yours.
…ow-tcp-connection-to-finish
…r reply with 200, else something is wrong.
…arangod processes
…db/ArangoDB into bug-fix/allow-tcp-connection-to-finish
…rangoDB into bug-fix/allow-tcp-connection-to-finish
…rangoDB into bug-fix/allow-tcp-connection-to-finish
…ow-tcp-connection-to-finish
…ow-tcp-connection-to-finish
|
tests green. |
…-fix/allow-tcp-connection-to-finish
No description provided.