[Gluster-devel] Glusterfs and the new logging framework

Mon Apr 28 09:42:56 UTC 2014

----- Original Message -----

> From: "Raghavendra Gowdappa" <rgowdapp at redhat.com>
> To: "Nithya Balachandran" <nbalacha at redhat.com>
> Cc: gluster-devel at gluster.org
> Sent: Monday, April 28, 2014 11:39:50 AM
> Subject: Re: [Gluster-devel] Glusterfs and the new logging framework

> (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);
> >
I am guessing the original author of the new logging framework decided to retain the msg string (like "Failed to get cached subvol for %s") 
in the header file due to the ease with which the one-to-one mapping between the msg id and the message itself can be captured using scripts/tools 
which may be needed for documentation - something that becomes difficult with the proposed approach. 

Even from development perspective, it is much easier for a developer adding new code (which means many more calls to gf_msg()) 
to merely look up one single place (which would be the messages header file for that component) to determine whether the message he/she is about to log 
is already in use (for example to know whether DHT_MSG_1 can be reused in their changes), rather than having to look up all files in that component. 

-Krutika 

> >
> > 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
> >
> >
> >
> >
> >
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://supercolony.gluster.org/mailman/listinfo/gluster-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://supercolony.gluster.org/pipermail/gluster-devel/attachments/20140428/0d80b7a8/attachment-0003.html>