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