rmdmattingly commented on code in PR #5335: URL: https://github.com/apache/hbase/pull/5335#discussion_r1278585995
########## hbase-server/src/main/resources/hbase-webapps/regionserver/rsOperationDetails.jsp: ########## @@ -25,6 +25,7 @@ import="org.apache.hadoop.util.StringUtils" import="org.apache.hadoop.hbase.regionserver.HRegionServer" import="org.apache.hadoop.hbase.HConstants" + import="org.apache.hadoop.hbase.client.OnlineLogRecord.deserializeAttributes" Review Comment: I don't know whether non-pretty-printed JSON is really what we want in the UI. We could create a String like we do for slow log params, but I think that's often hard to read too. At least with JSON one can copy & paste into a pretty printer if they really want to inspect in details. I think there might be a larger/separate ticket here regarding a refactor of the operation details table — for example: * I don't think the `Call Details` section is particularly useful. Rather than a toString representation of the request type, which can already be inferred by method type, we could consider making this a conglomerate payload of, for example, both request attributes, connection attributes, and any other call metadata that's appropriate. * we should omit table headers that are always empty in the given page. This is easier said than done and relatively low value I suppose, but it's not easy to read the current UI in my opinion. * we should omit `MultiX` columns that are not relevant to the payloads at hand. This, like omitting empty columns, wouldn't help a use-case with a wide variation in their slow logs' access patterns. I think this could be an argument for having separate, more readable, tables for each method type ########## hbase-server/src/main/resources/hbase-webapps/regionserver/rsOperationDetails.jsp: ########## @@ -25,6 +25,7 @@ import="org.apache.hadoop.util.StringUtils" import="org.apache.hadoop.hbase.regionserver.HRegionServer" import="org.apache.hadoop.hbase.HConstants" + import="org.apache.hadoop.hbase.client.OnlineLogRecord.deserializeAttributes" Review Comment: I don't know whether non-pretty-printed JSON is really what we want in the UI. We could create a String like we do for slow log params, but I think that's often hard to read too. At least with JSON one can copy & paste into a pretty printer if they really want to inspect in details. I think there might be a larger/separate ticket here regarding a refactor of the operation details table — for example: * I don't think the `Call Details` section is particularly useful. Rather than a toString representation of the request type, which can already be inferred by method type, we could consider making this a conglomerate payload of, for example, both request attributes, connection attributes, and any other call metadata that's appropriate. * we should omit table headers that are always empty in the given page. This is easier said than done and relatively low value I suppose, but it's not easy to read the current UI in my opinion. * we should omit `MultiX` columns that are not relevant to the payloads at hand. This, like omitting empty columns, wouldn't help a use-case with a wide variation in their slow logs' access patterns. I think this could be an argument for having separate, more readable, tables for each method type -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
