[
https://issues.apache.org/jira/browse/CASSANDRA-15766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17099691#comment-17099691
]
Berenguer Blasi edited comment on CASSANDRA-15766 at 5/5/20, 10:18 AM:
-----------------------------------------------------------------------
Great, Jira lost my comment. Here we go again...
I am a newbie here, so feel free to ignore me:
* I wonder why we don't defend with a {{wrapped.isLevelEnabled()}} at
{{NoSpamLogger}} top methods. It might pay off given we'd spare the many nested
method calls, the {{getStatement()}}, {{nanoTime()}}, {{shouldLog()}},... calls
* The {{Supplier}} approach is very nice, I like it a lot.
** If the overhead is negligible I would make that the only option. But given
my, maybe outdated, experiences with streams, lambdas, etc I am going to bet it
will be not :shrug:
** If it were not I'd rename top level methods as {{warnWithLazyParams()}} and
{{warnWithoutLazyParams()}}. If the dev is educated enough to have opted for
{{NoSpamLogger}} sure he will make the right choice here. I don't think we can
infer at call time which option is best, only the dev can.
+1 to a general 'other logs audit'. I would do it in another ticket as they
won't be in the hot path, they should be {{NoSpamLogger}} if they were, so that
is not as 'urgent' as this one imo.
Hope it makes sense. My 2cts
was (Author: bereng):
Great, Jira lost my comment. Here we go again...
I am a newbie here, so feel free to ignore me:
* I wonder why we don't defend with a {{wrapped.isLevelEnabled()}} at
{{NoSpamLogger}} top methods. It might pay off given we'd spare the many nested
method calls, the {{getStatement()}}, {{nanoTime()}}, {{shouldLog()},... calls
* The {{Supplier}} approach is very nice, I like it a lot.
** If the overhead is negligible I would make that the only option. But given
my, maybe outdated, experiences with streams, lambdas, etc I am going to bet it
will be not :shrug:
** If it were not I'd rename top level methods as {{warnWithLazyParams()}} and
{{warnWithoutLazyParams()}}. If the dev is educated enough to have opted for
{{NoSpamLogger}} sure he will make the right choice here. I don't think we can
infer at call time which option is best, only the dev can.
+1 to a general 'other logs audit'. I would do it in another ticket as they
won't be in the hot path, they should be {{NoSpamLogger}} if they were, so that
is not as 'urgent' as this one imo.
Hope it makes sense. My 2cts
> NoSpamLogger arguments building objects on hot paths
> ----------------------------------------------------
>
> Key: CASSANDRA-15766
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15766
> Project: Cassandra
> Issue Type: Bug
> Components: Observability/Logging
> Reporter: Jon Meredith
> Assignee: Jon Meredith
> Priority: Normal
> Fix For: 4.0-rc
>
>
> NoSpamLogger is used in hot logging paths to prevent logs being overrun. For
> that to be most effective the arguments to the logger need to be cheap to
> construct. During the internode messaging refactor CASSANDRA-15066,
> performance changes to BufferPool for CASSANDRA-14416
> were accidentally reverted in the merge up from 3.11.
> Reviewing other uses since, it looks like there are a few places where the
> arguments require some form of String building.
> org.apache.cassandra.net.InboundSink#accept
> org.apache.cassandra.net.InboundMessageHandler#processCorruptFrame
> org.apache.cassandra.net.InboundMessageHandler.LargeMessage#deserialize
> org.apache.cassandra.net.OutboundConnection#onOverloaded
> org.apache.cassandra.utils.memory.BufferPool.GlobalPool#allocateMoreChunks
> Formatting arguments should either be precomputed, or if expensive they
> should be computed after the decision on whether to noSpamLog has been made.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
