Update upon self review - mostly on comments

Author: renukamanavalan

common/events.h

@@ -6,12 +6,13 @@
  *
  *  APIs are for publishing & receiving events with source, tag and params along with timestamp.
  *  Used by event publishers and those interested in receiving published events.
- *  Publishers are multiple sources, as processes running in hosts & containers.
+ *  Publishers are multiple run from different contexts, as processes running in hosts & containers.
  *  Receiver are often few. Telmetry container runs a receiver.
  *
  */
 
 
+/* Handle for a publisher / subscriber instance */
 typedef void* event_handle_t;
 
 /*
@@ -20,8 +21,6 @@ typedef void* event_handle_t;
  *  A single publisher instance is maintained for a source.
  *  Any duplicate init call for a source will return the same instance.
  *
- *  Choosing cache will help read cached data, during downtime, if any.
- *
  * NOTE:
  *      The initialization occurs asynchronously.
  *      Any event published before init is complete, is blocked until the init
@@ -47,7 +46,7 @@ event_handle_t events_init_publisher(const std::string event_source);
  *  Handle returned from events_init_publisher
  *
  * Output: 
- *  None
+ *  Handle is nullified.
  */
 void events_deinit_publisher(event_handle_t &handle);
 
@@ -101,8 +100,6 @@ int event_publish(event_handle_t handle, const std::string event_tag,
 
 
 
-typedef std::vector<std::string> event_subscribe_sources_t;
-
 /*
  * Initialize subscriber.
  *  Init subscriber, optionally to filter by event-source.
@@ -112,6 +109,7 @@ typedef std::vector<std::string> event_subscribe_sources_t;
  *      When set to true, it will make use of the cache service transparently.
  *      The cache service caches events during session down time. The deinit
  *      start the caching and init call stops the caching.
+ *      default: false
  *
  * recv_timeout
  *      Read blocks by default until an event is available for read.
@@ -124,11 +122,14 @@ typedef std::vector<std::string> event_subscribe_sources_t;
  *      List of subscription sources of interest.
  *      The source value is the corresponding YANG module name.
  *      e.g. "sonic-events-bgp " is the source modulr name for bgp.
+ *      default: All sources, if none provided.
  *
  * Return:
  *  Non NULL handle on success
  *  NULL on failure
  */
+typedef std::vector<std::string> event_subscribe_sources_t;
+
 event_handle_t events_init_subscriber(bool use_cache=false,
         int recv_timeout = -1,
         const event_subscribe_sources_t *sources=NULL);
@@ -140,17 +141,18 @@ event_handle_t events_init_subscriber(bool use_cache=false,
  *  Handle returned from events_init_subscriber
  *
  * Output: 
- *  None
+ *  Handle is nullified.
  */
 void events_deinit_subscriber(event_handle_t &handle);
 
+
 /*
  * Receive an event.
- * A blocking call.
+ * A blocking call unless the subscriber is created with a timeout.
  *
  *  This API maintains an expected sequence number and use the received
  *  sequence in event to compute missed events count. The missed count
- *  set of events missed from this sender.
+ *  provides the count of events missed from this sender.
  *
  *  Received event:
  *      It is a form of JSON struct, with a single key and
@@ -198,18 +200,4 @@ int event_receive(event_handle_t handle, std::string &key,
  */
 int event_last_error();
 
-
-/*
- *  Cache drain timeout.
- *
- *  When de-init is called, it calls stop cache service.
- *  But before this point, there could be events received in zmq's
- *  local cache pending read and those that arrived since last read.
- *  These events will not be seen by cache service.
- *  So read those off and give it to cache service as starting stock.
- *  As we don't have a clue on count in zmq's cache, read in non-block
- *  mode for a period.
- */
-#define CACHE_DRAIN_IN_MILLISECS 1000
-
 #endif /* !_EVENTS_H */ 

common/events_common.h

@@ -29,16 +29,15 @@ extern int recv_last_err;
 
 /*
  * Max count of possible concurrent event publishers
- * A rough estimate only, more as a guideline than strict.
-         SWSS_LOG_ERROR(fmt.c_str(), e, zerrno __VA_OPT__(,) __VA_ARGS__); \
- * So this does not limit any usage
+ * We maintain a cache of last seen sequence number per publisher.
+ * This provides a MAX ceiling for cache.
+ * Any more publishers over this count should indicate a serious bug.
  */
 #define MAX_PUBLISHERS_COUNT  1000
 
 extern int running_ut;
 
 
-/* TODO: Combine two SWSS_LOG_ERROR into one */
 #define RET_ON_ERR(res, msg, ...)\
     if (!(res)) {\
         int _e = errno; \
@@ -50,9 +49,6 @@ extern int running_ut;
             printf("last:errno=%d zerr=%d\n", _e, zerrno); }\
         goto out; }
 
-#define ERR_CHECK(res, ...) {\
-    if (!(res)) \
-        SWSS_LOG_ERROR(__VA_ARGS__); }
 
 /* helper API to print variable type */
 /*
@@ -67,7 +63,6 @@ extern int running_ut;
  *    std::cout << type_name<decltype(t)>() << '\n';
  *    std::cout << type_name<decltype(tt_t)>() << '\n';
  */
-
 template <typename T> std::string type_name();
 
 template <class T>
@@ -102,6 +97,7 @@ get_typename(T &val)
 }
 
 
+/* map to human readable str; Useful for error reporting. */
 template <typename Map>
 string
 map_to_str(const Map &m)
@@ -170,8 +166,8 @@ const string get_timestamp();
  * Way to serialize map or vector
  * boost::archive::text_oarchive could be used to archive any struct/class
  * but that class needs some additional support, that declares
- * boost::serialization::access as private friend and couple more tweaks
- * std::map inherently supports serialization
+ * boost::serialization::access as private friend and couple more tweaks.
+ * The std::map & vector inherently supports serialization.
  */
 template <typename Map>
 int
@@ -249,9 +245,6 @@ zmsg_to_map(zmq_msg_t &msg, Map& data)
  * filter by source.
  *
  * Second part contains serialized form of map as defined in internal_event_t.
- * The map is serialized and sent as string events_data_type_t.
- * Caching that handles of set of events, handleas ordered events
- * as declared in events_data_lst_t.
  */
 /*
  * This is data going over wire and using cache. So be conservative
@@ -274,7 +267,7 @@ typedef string runtime_id_t;
  *    { EVENT_SEQUENCE, "" } };
  */
 
-/* ZMQ message part 2 contains serialized version of internal_event_t */
+/* Cache maintains the part 2 of an event as serialized string. */
 typedef string events_data_type_t;
 typedef vector<events_data_type_t> events_data_lst_t;
 

common/events_pi.h

@@ -130,4 +130,17 @@ class EventSubscriber : public events_base
 
 };
 
+/*
+ *  Cache drain timeout.
+ *
+ *  When de-init is called, it calls stop cache service.
+ *  But before this point, there could be events received in zmq's
+ *  local cache pending read and those that arrived since last read.
+ *  These events will not be seen by cache service.
+ *  So read those off and give it to cache service as starting stock.
+ *  As we don't have a clue on count in zmq's cache, read in non-block
+ *  mode for a period.
+ */
+#define CACHE_DRAIN_IN_MILLISECS 1000
+
 #endif /* !_EVENTS_SERVICE_H */