diff --git a/src/log/agent/lga_agent.cc b/src/log/agent/lga_agent.cc index 831edfa0f..1cb264e63 100644 --- a/src/log/agent/lga_agent.cc +++ b/src/log/agent/lga_agent.cc @@ -260,7 +260,7 @@ void LogAgent::RemoveAllDeadLogClients() { ScopeLock scopeLock(mutex_); auto it = client_list_.begin(); while (it != client_list_.end()) { - if ((*it)->is_died() == true) { + if ((*it)->is_dead() == true) { client_list_.erase(it); delete *it; it = client_list_.begin(); @@ -331,9 +331,9 @@ SaAisErrorT LogAgent::saLogInitialize(SaLogHandleT* logHandle, if (true) { ScopeLock critical_section(get_delete_obj_sync_mutex_); - if (has_dead_clients == true) { + if (has_dead_clients_ == true) { RemoveAllDeadLogClients(); - has_dead_clients = false; + has_dead_clients_ = false; } } @@ -476,7 +476,7 @@ SaAisErrorT LogAgent::saLogSelectionObjectGet( return ais_rc; } - if (client->is_died() == true) { + if (client->is_dead() == true) { ais_rc = SA_AIS_ERR_BAD_HANDLE; return ais_rc; } @@ -532,7 +532,7 @@ SaAisErrorT LogAgent::saLogDispatch(SaLogHandleT logHandle, return ais_rc; } - if (client->is_died() == true) { + if (client->is_dead() == true) { ais_rc = SA_AIS_ERR_BAD_HANDLE; return ais_rc; } @@ -617,9 +617,9 @@ SaAisErrorT LogAgent::saLogFinalize(SaLogHandleT logHandle) { if (true) { ScopeLock critical_section(get_delete_obj_sync_mutex_); - if (has_dead_clients == true) { + if (has_dead_clients_ == true) { RemoveAllDeadLogClients(); - has_dead_clients = false; + has_dead_clients_ = false; } // Get log client from the global list @@ -890,7 +890,7 @@ SaAisErrorT LogAgent::saLogStreamOpen_2( return ais_rc; } - if (client->is_died() == true) { + if (client->is_dead() == true) { ais_rc = SA_AIS_ERR_BAD_HANDLE; return ais_rc; } @@ -938,9 +938,9 @@ SaAisErrorT LogAgent::saLogStreamOpen_2( if (client->is_recovery_failed() == true) { ScopeLock critical_section(get_delete_obj_sync_mutex_); // Mark this client died, and it will be removed from client db later. - client->died(); + client->mark_dead(); client = nullptr; - has_dead_clients = true; + has_dead_clients_ = true; ais_rc = SA_AIS_ERR_BAD_HANDLE; return ais_rc; } @@ -1205,7 +1205,7 @@ SaAisErrorT LogAgent::saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, return ais_rc; } - if (client->is_died() == true) { + if (client->is_dead() == true) { ais_rc = SA_AIS_ERR_BAD_HANDLE; return ais_rc; } @@ -1316,10 +1316,10 @@ SaAisErrorT LogAgent::saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, if (client->is_recovery_failed() == true) { ScopeLock critical_section(get_delete_obj_sync_mutex_); // Mark this client died, and it will be removed from client db later. - client->died(); + client->mark_dead(); client = nullptr; stream = nullptr; - has_dead_clients = true; + has_dead_clients_ = true; ais_rc = SA_AIS_ERR_BAD_HANDLE; return ais_rc; } @@ -1382,7 +1382,7 @@ SaAisErrorT LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { return ais_rc; } - if (client->is_died() == true) { + if (client->is_dead() == true) { ais_rc = SA_AIS_ERR_BAD_HANDLE; return ais_rc; } @@ -1445,10 +1445,10 @@ SaAisErrorT LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { if (client->is_recovery_failed() == true) { ScopeLock critical_section(get_delete_obj_sync_mutex_); // Mark this client died, and it will be removed from client db later. - client->died(); + client->mark_dead(); client = nullptr; stream = nullptr; - has_dead_clients = true; + has_dead_clients_ = true; ais_rc = SA_AIS_ERR_BAD_HANDLE; return ais_rc; } diff --git a/src/log/agent/lga_agent.h b/src/log/agent/lga_agent.h index 817a357ec..016101a2b 100644 --- a/src/log/agent/lga_agent.h +++ b/src/log/agent/lga_agent.h @@ -33,6 +33,12 @@ class LogStreamInfo; class LogClient; +// [Lennart] I find some of what's written here hard to understand and some +// information I want to see here is missing so I have to analyze the code +// to understand what it is doing and how it is used or should be used +// I have therefore given some comments based some fast analyzing and in some +// cases I may have missed something. Please improve. + //> // @LogAgent class - singleton // @@ -43,7 +49,14 @@ class LogClient; // @LogAgent singleton object is created when starting LOG application. // When @saLogInitialize() API is called successfully, one @LogClient object // is created and added to a list @client_list_, and removed when that client -// is finalized. All operation on the lists is protected by mutex. +// is finalized. All operations on the list is protected by mutex. +// [Lennart] I think that clients are removed for other reasons as well e.g. +// If we have been head-less and if after all possible recovery is done a +// stream that is used by a client could not be restored? +// Also saying that the list is protected by mutex is not interesting to know +// here. It is more informative to inform here that all methods used to handle +// the list is thread safe (if that is really the case) and if there are any +// exemptions. // // When getting @NCSMDS_DOWN event from MDS thread, @NoLogServer() method // will be called by MDS thread to inform the situation to @LogAgent. @@ -52,12 +65,30 @@ class LogClient; // to all its @LogStreamInfo. If getting the @NCSMDS_DOWN event // all on-going recovery clients will be canceled immediately. // -// To deal with the case @LogClient or @LogStreamInfo object is deleted -// while that objects are being referred by somewhere else, these objects -// have their one own attribute, called @ref_counter_. The attribute has one -// of these 03 status: being use (>0), being delete(-1) or none of above - idle. +// [Lennart] Broadcast?? A log agent is a library used by a process and all +// clients in its list of clients are handled in the process using the library +// so no broadcast is needed. And again, when reading here I am not interested +// in how the code does its job, I am interested in WHAT the code is supposed +// to do. The code itself shall be written in such a way that it by itself, +// maybe with help of some comments describes how it does what's described here +// What may be needed is to set some flag for each +// client to handle whether the client is available or not. For an unavailable +// client (some APIs (not all) shall return TRY AGAIN if not available). After +// a successful recovery APIs shall work normally again. A successful recovery +// means that all streams used by a client are recovered. +// Note that it is actually a bit misleading to say that a client is recovered +// even it is somewhat true. What is really recovered is the log streams that +// where open when we became headless. +// +// To prohibit a @LogClient or @LogStreamInfo object to be deleted while being +// referred, these objects have a reference counter @ref_counter_. +// The attribute has one of these 03 status: being use (>0), being delete(-1) +// or 0. +// [Lennart] This is not a clean reference counter but a combination of +// reference counter and state variable with three states "in use", +// "being deleted" and "idle", so call it a reference counter is misleading. // Once the LOG client wants to delete the object (e.g: Deleting log stream) -// but it is in state "being use", @LogAgent will not allow to delete +// but it is in state "in use", @LogAgent will not allow deleting the object, // that object, but saying try next time until its state is in "idle" (0). // Or in other case, if the object is marked "being delete" // then, any operation on that log stream object will be rejected. @@ -156,9 +187,22 @@ class LogAgent { // Enter critical section - make sure ref counter is fetched. // Introduce these public interface for MDS thread use. + // [Lennart] Why is this needed, at least as public methods?? + // Why are not all methods handling variables, lists etc. used in several + // threads thread safe?? void EnterCriticalSection(); void LeaveCriticalSection(); + // [Lennart] Dead clients is not a descriptive name. It is not possible to + // know what a dead client is without analyzing the code + // As far as I can see a "dead" client is a client that has a log stream that + // could not be restored after head-less. + // Suggest change to for example RemoveAllRecoveryFailedClients() + // Also add an explanation in for example the class information. + // I assume that the reason for having this method and the corresponding + // "dead flags" is that the client may be referenced by a stream and that + // the client refers to that stream. This "double" reference should be + // preferable be removed, may however be too much refactoring for now. void RemoveAllDeadLogClients(); private: @@ -275,7 +319,7 @@ class LogAgent { std::vector client_list_; // true if there is any failed-to-recover client - bool has_dead_clients = false; + bool has_dead_clients_ = false; // LGS LGA sync params NCS_SEL_OBJ lgs_sync_sel_; diff --git a/src/log/agent/lga_client.cc b/src/log/agent/lga_client.cc index 3c3f89366..4e18bd49e 100644 --- a/src/log/agent/lga_client.cc +++ b/src/log/agent/lga_client.cc @@ -32,7 +32,7 @@ // LogClient //------------------------------------------------------------------------------ LogClient::LogClient(const SaLogCallbacksT* cb, uint32_t id, SaVersionT ver) - : client_id_{id}, ref_counter_object_{}, died_{false} { + : client_id_{id}, ref_counter_object_{}, dead_{false} { TRACE_ENTER(); // Reset registered callback info memset(&callbacks_, 0, sizeof(callbacks_)); diff --git a/src/log/agent/lga_client.h b/src/log/agent/lga_client.h index 08137b7a9..bcdfb4ab5 100644 --- a/src/log/agent/lga_client.h +++ b/src/log/agent/lga_client.h @@ -174,8 +174,9 @@ class LogClient { bool is_recovery_done() const; // Invoke when the client is not recovered successfully. - void died() { died_ = true; } - bool is_died() const { return died_; } + // [Lennart] Find a better name. I have changed the names still using dead + void mark_dead() { dead_ = true; } + bool is_dead() const { return dead_; } private: // Search for @LogStreamInfo object from @stream_list_ based on @stream_id @@ -262,7 +263,7 @@ class LogClient { SaLogHandleT handle_; // true if this client is not successfully recovered. - bool died_; + bool dead_; DELETE_COPY_AND_MOVE_OPERATORS(LogClient); };