[Gluster-devel] Glusterfs and the new logging framework

[Raghavendra Gowdappa]

(Moving the discussion to gluster-devel).

----- Original Message -----
> From: "Nagaprasad Sathyanarayana" <nsathyan at redhat.com>
> To: "Nithya Balachandran" <nbalacha at redhat.com>
> Cc: "Raghavendra Gowdappa" <rgowdapp at redhat.com>, "Susant Palai" <spalai at redhat.com>, "Atin Mukherjee"
> <amukherj at redhat.com>, "Varun Shastry" <vshastry at redhat.com>, "Krishnan Parthasarathi" <kparthas at redhat.com>,
> "Ravishankar Narayanankutty" <ranaraya at redhat.com>, "Pranith Kumar Karampuri" <pkarampu at redhat.com>, "Venkatesh
> Somyajulu" <vsomyaju at redhat.com>
> Sent: Monday, April 28, 2014 10:53:55 AM
> Subject: Re: Glusterfs and the new logging framework
> 
> It is a very valuable suggestion Nithya.  Using descriptive message IDs
> (DHT_GET_CACHED_SUBVOL_FAILED in place of dht_msg_1) is definitely helpful
> to developers and improves the code readability.  However, IMO, keeping the
> actual formatted message string in a header file is a good practice.  Some
> of the reasons for it are;
> 
> A> It helps in internationalization and localization at later point.
> B> Any changes to the message & the formatting is easy to handle when it is
> not embedded in the log function call.
> 
> Please discuss this with wider audience, including Vijay & Alok, before
> finalizing the approach.
> 
> Thanks
> Naga
> 
> ----- Original Message -----
> From: "Nithya Balachandran" <nbalacha at redhat.com>
> To: "Raghavendra Gowdappa" <rgowdapp at redhat.com>, "Susant Palai"
> <spalai at redhat.com>, "Atin Mukherjee" <amukherj at redhat.com>, "Varun Shastry"
> <vshastry at redhat.com>, "Krishnan Parthasarathi" <kparthas at redhat.com>,
> "Ravishankar Narayanankutty" <ranaraya at redhat.com>, "Pranith Kumar
> Karampuri" <pkarampu at redhat.com>, "Venkatesh Somyajulu"
> <vsomyaju at redhat.com>
> Cc: "Nagaprasad Sathyanarayana" <nsathyan at redhat.com>
> Sent: Monday, April 28, 2014 9:16:37 AM
> Subject: Glusterfs and the new logging framework
> 
> Hi,
> 
> I am currently working on porting the DHT messages to the new logging
> framework and I have some observations/concerns about the approach on the
> same:
> 
> 1. With the current approach, a call to gf_msg would look like:
> 
> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_1, param1);
> 
> This provides little information to the developer when going through the
> code. It becomes tedious to lookup the value of dht_msg_4 each time. This
> also reduces code readability as the format string is not available
> instantly and can potentially cause issues with incorrect parameters being
> passed to the call.
> 
> 
> 2. Adding new messages can potentially cause us to use up the chunk of
> message ids quickly. A better approach would be to decouple the is
> definition from the actual message string.There are be several potential
> messages for a single operation state.Message formats might change in the
> future as we add enhancements - for eg, we shouldn't have to add new
> messages with new message ids to accommodate an additional parameter if we
> change a struct definition, say. Defining some sort of "state ids" however
> will take up fewer ids.
> 
> Eg: The code has the following messages :
> 
> "Failed to get cached subvol for %s"
> "Failed to get cached subvol for %s on %s"
> 
> 
> Both the above will require separate msg-ids with the current approach. The
> difference in the strings is minimal and does not really provide any
> additional information from the Doxygen doc point of view.
> 
> Another approach would be to define a DHT_GET_CACHED_SUBVOL_FAILED id which
> can be passed to the msglog, while the actual message string can change
> depending on what the developer thinks will be helpful. This will also
> improve code readability.
> 
> So I propose that we do the following:
> 
> 
> Current approach:
> 
> Messages file:
> 
> #define dht_msg_1 (GLFS_COM_BASE + 1) , "Failed to get cached subvol for %s"
> #define dht_msg_2 (GLFS_COM_BASE + 2) , "Failed to get cached subvol for %s
> on %s"
> #define dht_msg_3 (GLFS_COM_BASE + 3) , "Failed to get hashed subvol " ...etc
> etc
> 
> Code :
> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_1, param1);
> 
> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_2, param1, param2);
> 
> The above is confusing for a developer.
> 
> 
> Proposed approach:
> 
> #define DHT_GET_CACHED_SUBVOL_FAILED (GLFS_COM_BASE + 1)
> #define DHT_GET_HASHED_SUBVOL_FAILED (GLFS_COM_BASE + 2)
> 
> gf_msg ("dht", GF_LOG_ERROR, 0, DHT_GET_CACHED_SUBVOL_FAILED, "Failed to get
> cached subvol for %s on %s" , param1, param2);
> 
> gf_msg ("dht", GF_LOG_ERROR, 0, DHT_GET_CACHED_SUBVOL_FAILED, "Failed to get
> cached subvol for %s " , param1);
> 
> 
> Advantages:
> Much clearer code - developer can use generated id or define new one easily.
> Related ids can be grouped together in the header file.
> Fewer ids required
> No changes to the logging framework
> Messages can change later
> The approach of using and id and generating a doxygen doc for the same still
> holds good - we are simply decoupling the actual string from the definition.
> So the doc would still have the message id and a description of the
> condition it specifies without tying it to a string that might change later.
> 
> 
> Thoughts?
> 
> Regards,
> Nithya
> 
> 
> 
> 
>