<html><body><div style="font-family: garamond,new york,times,serif; font-size: 12pt; color: #000000"><div><br></div><div><br></div><hr id="zwchr"><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><b>From: </b>"Raghavendra Gowdappa" <rgowdapp@redhat.com><br><b>To: </b>"Nithya Balachandran" <nbalacha@redhat.com><br><b>Cc: </b>gluster-devel@gluster.org<br><b>Sent: </b>Monday, April 28, 2014 11:39:50 AM<br><b>Subject: </b>Re: [Gluster-devel] Glusterfs and the new logging framework<br><div><br></div>(Moving the discussion to gluster-devel).<br><div><br></div>----- Original Message -----<br>> From: "Nagaprasad Sathyanarayana" <nsathyan@redhat.com><br>> To: "Nithya Balachandran" <nbalacha@redhat.com><br>> Cc: "Raghavendra Gowdappa" <rgowdapp@redhat.com>, "Susant Palai" <spalai@redhat.com>, "Atin Mukherjee"<br>> <amukherj@redhat.com>, "Varun Shastry" <vshastry@redhat.com>, "Krishnan Parthasarathi" <kparthas@redhat.com>,<br>> "Ravishankar Narayanankutty" <ranaraya@redhat.com>, "Pranith Kumar Karampuri" <pkarampu@redhat.com>, "Venkatesh<br>> Somyajulu" <vsomyaju@redhat.com><br>> Sent: Monday, April 28, 2014 10:53:55 AM<br>> Subject: Re: Glusterfs and the new logging framework<br>> <br>> It is a very valuable suggestion Nithya. Using descriptive message IDs<br>> (DHT_GET_CACHED_SUBVOL_FAILED in place of dht_msg_1) is definitely helpful<br>> to developers and improves the code readability. However, IMO, keeping the<br>> actual formatted message string in a header file is a good practice. Some<br>> of the reasons for it are;<br>> <br>> A> It helps in internationalization and localization at later point.<br>> B> Any changes to the message & the formatting is easy to handle when it is<br>> not embedded in the log function call.<br>> <br>> Please discuss this with wider audience, including Vijay & Alok, before<br>> finalizing the approach.<br>> <br>> Thanks<br>> Naga<br>> <br>> ----- Original Message -----<br>> From: "Nithya Balachandran" <nbalacha@redhat.com><br>> To: "Raghavendra Gowdappa" <rgowdapp@redhat.com>, "Susant Palai"<br>> <spalai@redhat.com>, "Atin Mukherjee" <amukherj@redhat.com>, "Varun Shastry"<br>> <vshastry@redhat.com>, "Krishnan Parthasarathi" <kparthas@redhat.com>,<br>> "Ravishankar Narayanankutty" <ranaraya@redhat.com>, "Pranith Kumar<br>> Karampuri" <pkarampu@redhat.com>, "Venkatesh Somyajulu"<br>> <vsomyaju@redhat.com><br>> Cc: "Nagaprasad Sathyanarayana" <nsathyan@redhat.com><br>> Sent: Monday, April 28, 2014 9:16:37 AM<br>> Subject: Glusterfs and the new logging framework<br>> <br>> Hi,<br>> <br>> I am currently working on porting the DHT messages to the new logging<br>> framework and I have some observations/concerns about the approach on the<br>> same:<br>> <br>> 1. With the current approach, a call to gf_msg would look like:<br>> <br>> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_1, param1);<br>> <br>> This provides little information to the developer when going through the<br>> code. It becomes tedious to lookup the value of dht_msg_4 each time. This<br>> also reduces code readability as the format string is not available<br>> instantly and can potentially cause issues with incorrect parameters being<br>> passed to the call.<br>> <br>> <br>> 2. Adding new messages can potentially cause us to use up the chunk of<br>> message ids quickly. A better approach would be to decouple the is<br>> definition from the actual message string.There are be several potential<br>> messages for a single operation state.Message formats might change in the<br>> future as we add enhancements - for eg, we shouldn't have to add new<br>> messages with new message ids to accommodate an additional parameter if we<br>> change a struct definition, say. Defining some sort of "state ids" however<br>> will take up fewer ids.<br>> <br>> Eg: The code has the following messages :<br>> <br>> "Failed to get cached subvol for %s"<br>> "Failed to get cached subvol for %s on %s"<br>> <br>> <br>> Both the above will require separate msg-ids with the current approach. The<br>> difference in the strings is minimal and does not really provide any<br>> additional information from the Doxygen doc point of view.<br>> <br>> Another approach would be to define a DHT_GET_CACHED_SUBVOL_FAILED id which<br>> can be passed to the msglog, while the actual message string can change<br>> depending on what the developer thinks will be helpful. This will also<br>> improve code readability.<br>> <br>> So I propose that we do the following:<br>> <br>> <br>> Current approach:<br>> <br>> Messages file:<br>> <br>> #define dht_msg_1 (GLFS_COM_BASE + 1) , "Failed to get cached subvol for %s"<br>> #define dht_msg_2 (GLFS_COM_BASE + 2) , "Failed to get cached subvol for %s<br>> on %s"<br>> #define dht_msg_3 (GLFS_COM_BASE + 3) , "Failed to get hashed subvol " ...etc<br>> etc<br>> <br>> Code :<br>> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_1, param1);<br>> <br>> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_2, param1, param2);<br>> <br>> The above is confusing for a developer.<br>> <br>> <br>> Proposed approach:<br>> <br>> #define DHT_GET_CACHED_SUBVOL_FAILED (GLFS_COM_BASE + 1)<br>> #define DHT_GET_HASHED_SUBVOL_FAILED (GLFS_COM_BASE + 2)<br>> <br>> gf_msg ("dht", GF_LOG_ERROR, 0, DHT_GET_CACHED_SUBVOL_FAILED, "Failed to get<br>> cached subvol for %s on %s" , param1, param2);<br>> <br>> gf_msg ("dht", GF_LOG_ERROR, 0, DHT_GET_CACHED_SUBVOL_FAILED, "Failed to get<br>> cached subvol for %s " , param1);<br>> </blockquote><div>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")</div><div>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</div><div>which may be needed for documentation - something that becomes difficult with the proposed approach.<br></div><div><br></div><div>Even from development perspective, it is much easier for a developer adding new code (which means many more calls to gf_msg())</div><div>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<br></div><div>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.<br></div><div><br></div><div>-Krutika<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;">> <br>> Advantages:<br>> Much clearer code - developer can use generated id or define new one easily.<br>> Related ids can be grouped together in the header file.<br>> Fewer ids required<br>> No changes to the logging framework<br>> Messages can change later<br>> The approach of using and id and generating a doxygen doc for the same still<br>> holds good - we are simply decoupling the actual string from the definition.<br>> So the doc would still have the message id and a description of the<br>> condition it specifies without tying it to a string that might change later.<br>> <br>> <br>> Thoughts?<br>> <br>> Regards,<br>> Nithya<br>> <br>> <br>> <br>> <br>> <br>_______________________________________________<br>Gluster-devel mailing list<br>Gluster-devel@gluster.org<br>http://supercolony.gluster.org/mailman/listinfo/gluster-devel<br></blockquote><div><br></div></div></body></html>