You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/05/04 23:46:54 UTC
[GitHub] [cassandra] jonmeredith opened a new pull request #582: Add support for lazy NoSpamLogger argument evaluation
jonmeredith opened a new pull request #582:
URL: https://github.com/apache/cassandra/pull/582
NoSpamLogger is use in hot logging paths. To get the greatest benefit
calling should be as cheap as possible. This patch adds a variant to
NoSpamLogger.log and friends to lazily supply the formatting parameters.
It also introduces a params() helper function to make constructing the
parameter array supplier cleaner.
Fixed up existing call sites to use params where it made sense and
altered the logging for org.apache.cassandra.net.InboundSink#accept
so that log statements for tombstones and indices would be output
correctly. The previous getMessage format string included info on
keys, tables and queries that would vary between calls.
Patch by Jon Meredith; reviewed by ? for CASSANDRA-15766.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith closed pull request #582: Add support for lazy NoSpamLogger argument evaluation
Posted by GitBox <gi...@apache.org>.
jonmeredith closed pull request #582:
URL: https://github.com/apache/cassandra/pull/582
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a change in pull request #582: Add support for lazy NoSpamLogger argument evaluation
Posted by GitBox <gi...@apache.org>.
maedhroz commented on a change in pull request #582:
URL: https://github.com/apache/cassandra/pull/582#discussion_r454108480
##########
File path: src/java/org/apache/cassandra/net/InboundSink.java
##########
@@ -96,8 +98,10 @@ public void accept(Message<?> message)
{
fail(message.header, t);
- if (t instanceof TombstoneOverwhelmingException || t instanceof IndexNotAvailableException)
- noSpamLogger.error(t.getMessage());
Review comment:
@jonmeredith What if we just extended `NoSpamLogger` to take a properly typed `Throwable` and pass that to the underlying `Logger`? It would avoid the lambda creation at least...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a change in pull request #582: Add support for lazy NoSpamLogger argument evaluation
Posted by GitBox <gi...@apache.org>.
maedhroz commented on a change in pull request #582:
URL: https://github.com/apache/cassandra/pull/582#discussion_r454108480
##########
File path: src/java/org/apache/cassandra/net/InboundSink.java
##########
@@ -96,8 +98,10 @@ public void accept(Message<?> message)
{
fail(message.header, t);
- if (t instanceof TombstoneOverwhelmingException || t instanceof IndexNotAvailableException)
- noSpamLogger.error(t.getMessage());
Review comment:
@jonmeredith What if we just extended `NoSpamLogger` to take a properly typed `Throwable` and pass that to the underlying `Logger`? It would avoid the lambda creation at least...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a change in pull request #582: Add support for lazy NoSpamLogger argument evaluation
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on a change in pull request #582:
URL: https://github.com/apache/cassandra/pull/582#discussion_r454439858
##########
File path: src/java/org/apache/cassandra/utils/memory/BufferPool.java
##########
@@ -232,8 +236,8 @@ private Chunk allocateMoreChunks()
if (MEMORY_USAGE_THRESHOLD > 0)
{
noSpamLogger.info("Maximum memory usage reached ({}), cannot allocate chunk of {}",
- prettyPrintMemory(MEMORY_USAGE_THRESHOLD),
- prettyPrintMemory(MACRO_CHUNK_SIZE));
+ () -> params(prettyPrintMemory(MEMORY_USAGE_THRESHOLD),
+ prettyPrintMemory(MACRO_CHUNK_SIZE)));
Review comment:
I think pre-computing them is a good plan where constant or they are set infrequently. The `sizeInBytes()` in the failed allocation will be variable, but fixing all the other occurrences would definitely be a good thing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a change in pull request #582: Add support for lazy NoSpamLogger argument evaluation
Posted by GitBox <gi...@apache.org>.
maedhroz commented on a change in pull request #582:
URL: https://github.com/apache/cassandra/pull/582#discussion_r454099460
##########
File path: src/java/org/apache/cassandra/utils/memory/BufferPool.java
##########
@@ -232,8 +236,8 @@ private Chunk allocateMoreChunks()
if (MEMORY_USAGE_THRESHOLD > 0)
{
noSpamLogger.info("Maximum memory usage reached ({}), cannot allocate chunk of {}",
- prettyPrintMemory(MEMORY_USAGE_THRESHOLD),
- prettyPrintMemory(MACRO_CHUNK_SIZE));
+ () -> params(prettyPrintMemory(MEMORY_USAGE_THRESHOLD),
+ prettyPrintMemory(MACRO_CHUNK_SIZE)));
Review comment:
@jonmeredith Possibly naive question...why don't we just pre-compute human readable versions of `MEMORY_USAGE_THRESHOLD` and `MACRO_CHUNK_SIZE` (using `prettyPrintMemory()`)? There are some tests that write to `MEMORY_USAGE_THRESHOLD` of course, but we can get a `static` setter in place that updates its "companion" as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on pull request #582: Add support for lazy NoSpamLogger argument evaluation
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on pull request #582:
URL: https://github.com/apache/cassandra/pull/582#issuecomment-778314333
alternate version merged
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org