[jira] [Commented] (CASSANDRA-9692) Print sensible units for all log messages

["Joel Knighton (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-9692?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15191630#comment-15191630 ] 

Joel Knighton commented on CASSANDRA-9692:
------------------------------------------

Thanks [~giampaolo]! This looks good overall and very thorough.

A few comments to fix up before we get this merged:
* In CompactionManager, we shouldn't pretty print the expected bloom filter size. This size is in entries, not bytes.
* Is there a strong reason to change the IOException to a RuntimeException in RangeAwareSSTableWriter? I'd undo that change.
* There's a small typo in the format string for "Insufficient disk space" in StreamReader.
* A parenthesis is missing in IndexSummaryRedistribution.
* A comma is missing in the argument list in BufferPool.
* In BulkLoader, we need to append a parenthesis at the end of (avg: ). We should also add a space in front. The change to the padding of the percentage fields is unnecessary, and we also don't need to pad the rate in seconds.

The details of these suggested tweaks are here: [review comments|https://github.com/jkni/cassandra/commit/290adc8d1f1f1644e5a886dace857430a0f2f958].

There are a few other changes I'd suggest but haven't done:
* The prettyPrintRateInSeconds method seems useful - I think we should move it to FBUtilities like prettyPrintMemory. Renaming the argument from size to rate would make more sense to me.
* It seems like we should have consistency in printing between prettyPrintRateInSeconds (X.XX YiB/s) and prettyPrintMemory (X.XXXYiB). I'd prefer the format of prettyPrintMemory, where we add another digit and remove the space.
* In CompactionTask, we may want to convert the rate in {{runMayThrow}} and the total bytes compacted in {{runMayThrow}}.

Rebasing on trunk before pushing a patch addressing these concerns would be great. Once we have a patch we agree on, I'll run it in CI so we can confirm that this doesn't break any tests.

> Print sensible units for all log messages
> -----------------------------------------
>
>                 Key: CASSANDRA-9692
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-9692
>             Project: Cassandra
>          Issue Type: Improvement
>            Reporter: Benedict
>            Assignee: Giampaolo
>            Priority: Minor
>              Labels: lhf
>             Fix For: 3.x
>
>         Attachments: Cassandra9692-trunk-giampaolo-trapasso-at-radicalbit-io.diff
>
>
> Like CASSANDRA-9691, this has bugged me too long. it also adversely impacts log analysis. I've introduced some improvements to the bits I touched for CASSANDRA-9681, but we should do this across the codebase. It's a small investment for a lot of long term clarity in the logs.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)