You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mahesh Reddy (Code Review)" <ge...@cloudera.org> on 2020/09/02 00:37:50 UTC
[kudu-CR] KUDU-3012: Log Throttler
Mahesh Reddy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16400
Change subject: KUDU-3012: Log Throttler
......................................................................
KUDU-3012: Log Throttler
ThrottlerLogUtil uses instance of Logger and only logs one message per a
time period that can be set at the callsite. Implementation uses a
ConcurrentHashMap to track messages and relevant part of timestamps.
Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
3 files changed, 186 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/16400/1
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16400/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS3, Line 118: final
Should this be static?
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java@46
PS2, Line 46: :
> actually the colon or any character is necessary after i because if I remov
Alternatively, you can only throttle 10 messages.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 19:41:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS5, Line 554: 60L
> In this particular case, would it be enough to log just once regardless of
I think we wan't to log it periodically to make sure it's seen. Logs can roll and often administrators only look at the tail of a log.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 13:53:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@34
PS5, Line 34: TODO(mreddy): Efficiently support variable arguments by explicitly checking for arguments length
: * in logging functions and call corresponding function rather than calling function with varargs
: * every time. This will avoid the hidden cost of creating an Object[] before invoking the method.
> good point, took out the varargs support for now and put it in a TODO
The only concern with variadic arguments is performance, right?
Lets say another log message which we think should be throttled shows up soon and that takes arguments and not just message, wouldn't it be convenient to simply use this LogThrottle class instead of re-implementing stuff that Mahesh has already implemented?
IMO it's better to keep the variadic argument support and tackle performance concerns once it really becomes one.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 14:47:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Log Throttler
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/16400/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16400/1//COMMIT_MSG@7
PS1, Line 7: Log Throttler
Nit: Add a Java Log Throttler.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS1, Line 118: logThrottler
It's a static final object, so should be named in caps like THROTTLE_LOG
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@28
PS1, Line 28: // Issue 1: store template message w/ or w/o parameters, if just template message then assuming failure is a
: // parameter if something fails once then a second time but this time b/c of a different reason, second log message
: // will be suppressed and we won't know that there's a second cause of this failure
: //
: // Issue 2: Use log.info(msg, arguments) for everything or check for length so Object[] for arguments wont be
: // created before method is invoked. From the Logger Interface regarding this issue:
: // However, this variant incurs the hidden (and relatively small) cost of creating an <code>Object[]</code> before
: // invoking the method, even if this logger is disabled for INFO.
: //
: // Note for Issue 2: Only using INFO level as an example, applies to other Levels too
These lines look long. Line length needs to be < 100.
Java style checker would point them out.
Same for bunch of lines below.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50
PS1, Line 50: static
As per the current implementation, I don't think this should be static which means the same map will be used by all instances of throttled logger.
Also should we consider limiting the size of the map to prevent it from growing too large?
Currently once added the log entries are never removed.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50
PS1, Line 50: final
Values are added to this map, so this can't be final.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@52
PS1, Line 52:
Nit: Unnecessary space.
I'll let the style checker point of such issues.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@62
PS1, Line 62: long seconds
How about using Duration as argument for all variants so that callers don't have to do computation for minutes, milliseconds etc.?
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@123
PS1, Line 123: processing
Choose a better name as "processing" doesn't convey much, like shouldLog()?
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@123
PS1, Line 123: message
Like other functions, msg should be sufficient.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 01:01:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@42
PS2, Line 42: ThrottlerLogUtil
> nit: Maybe name this "LogThrottler" now that it's not a utility class?
Done
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java@46
PS2, Line 46: :
> nothing really, I can remove it
actually the colon or any character is necessary after i because if I remove it, the test will fail because "Logging 5" is seen as a part of "Logging 51" without the colon and that would fail the assertFalse statement as only "Logging 51" should be logged while "Logging 5" shouldn't be logged.
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java@56
PS2, Line 56:
> hm but checkstyle didnt complain? it's a continuation of the previous line
Done
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 18:22:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16400
to look at the new patch set (#6).
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
KUDU-3012: Add a Log Throttler
LogThrottler implements a simple log throttler by logging at most one
message per a time period that can be set at the call-site. Each
instance of this class is designed to throttle regardless of message.
Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
3 files changed, 190 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/16400/6
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3012: Log Throttler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler
......................................................................
Patch Set 1:
(6 comments)
Just passing through.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
PS1:
Could probably use some scrubbing to match the GSG https://google.github.io/styleguide/javaguide.html
Or at least match existing codebase style.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50
PS1, Line 50: timestamps
nit: this isn't just the timestamps, so its call-sites read somewhat awkwardly IMO. It'd be a bit more self-documenting if it were named timestampSecsByMessage or messageToTimestampSecs or lastLoggedTimeSecsPerMessage or something?
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@63
PS1, Line 63: processing(seconds, msg)
Depending on how pervasive this becomes, rather than mapping the message to lastLoggedTimestamp, it might be worth considering mapping some integer key instead, and exposing the keys publicly, e.g.
ThrottlerLogUtil {
public static final int APPLY_ON_CLOSED_KEY = 0;
public static final int OTHER_THROTTLED_KEY = 1;
public void info(long seconds, int key, String msg, ...) {
if (processing(seconds, key)) {
log.info(msg, ...);
}
}
}
and at call-sites:
logThrottler.info(10, APPLY_ON_CLOSED_KEY, "message");
That would also allow the message to contain mutable messages, if we ever want to use this for that. What do you think?
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@119
PS1, Line 119: @param duration number of seconds between each desired log message
nit: How about calling this something like throttlingIntervalSecs or somesuch? That way it's clear from the name what this is and how it's used.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@124
PS1, Line 124: current
nit: maybe "nowSecs"?
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@125
PS1, Line 125: // probably can just use duration instead of gap
What is is this conversion doing? Isn't 'time' already TimeUnit.SECONDS?
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 01:36:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16400/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16400/5//COMMIT_MSG@7
PS5, Line 7: KUDU-3012: Add a Log Throttler
As an alternative approach, could it be enough to log that particular warning message just once?
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS5, Line 554: 60L
In this particular case, would it be enough to log just once regardless of how long a closed session being pounded with apply() calls? Then there is no need to have log throttler at all.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Sep 2020 22:34:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16400
to look at the new patch set (#7).
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
KUDU-3012: Add a Log Throttler
LogThrottler implements a simple log throttler by logging at most one
message per a time period that can be set at the call-site. Each
instance of this class is designed to throttle regardless of message.
Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
3 files changed, 384 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/16400/7
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Sep 2020 00:30:36 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG@9
PS4, Line 9: LogThrottler uses an instance of Logger
This is part of implementation detail and unnecessary.
Better to mention that this change implements a simple log throttler that throttles messages by logging at most one log message per specified time duration.
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG@11
PS4, Line 11: message.
message parameters, right?
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS4, Line 118: final
We want a single instance per class, so this should be static.
If it's static final then it should be named in CAPS.
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS4, Line 554: 10
I think 60 secs would be better.
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@42
PS4, Line 42: private long lastLoggedTime = 0;
Nit: If time were to start from epoch then the first log message will be throttled :)
One option would be to initialize with -1 and use that special value in shouldLog().
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@49
PS4, Line 49: * Calls shouldLog method, logs message at trace level if method true, does nothing if false
Java doc comments are meant for the users and there is no need to mention private function names.
Same for methods below.
Something like...
"Throttles the log trace message 'msg' if the last message was logged less than 'seconds' ago."
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 22:48:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@550
PS2, Line 550: ThrottlerLogUtil throttleLog = new ThrottlerLogUtil(LOG);
> If we instantiate a new ThrottlerLogUtil every time apply() is called, won'
ah yeh oversight on my part will fix it in next patch tomorrow
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@29
PS2, Line 29: TODO:
> nit: can you suffix this with your id, same below, e.g.
Done
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@29
PS2, Line 29: If functionality is ever expanded, use ConcurrentHashMap to store multiple messages and
: * the last time it was logged, only one instance of ThrottlerLogUtil will be needed per class then,
: * use integer as key rather than string for performance costs
> nit: this can probably be broken into a couple sentences, given you are des
Done
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java@46
PS2, Line 46: :
> nit: what is the point of this colon?
nothing really, I can remove it
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java@56
PS2, Line 56:
> nit: this seems like too many spaces. Same below.
hm but checkstyle didnt complain? it's a continuation of the previous line since it was too long to fit within 100 characters so maybe that's why it wrapped the next line like that
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 06:44:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG@11
PS4, Line 11: message.
> not sure I understand
throttles regardless of message
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS4, Line 118: final
> I think we want to throttle per-session in this case, but I could go either
decided to keep it as is for now, if a strong case for making it static is made can change it
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS4, Line 554: 10
> even if that means only log one message per 60 secs?
decided once per minute is fine
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 22:02:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS4, Line 118: final
> Then multiple instances of AsyncKuduSession will share the same throttler a
I think we want to throttle per-session in this case, but I could go either way.
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@42
PS4, Line 42: private long lastLoggedTime = 0;
> Done
I think you need to push the latest changes.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 14:04:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler
......................................................................
Patch Set 1:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/16400/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16400/1//COMMIT_MSG@7
PS1, Line 7: Log Throttler
> Nit: Add a Java Log Throttler.
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS1, Line 118: logThrottler
> It's a static final object, so should be named in caps like THROTTLE_LOG
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
PS1:
> Could probably use some scrubbing to match the GSG https://google.github.io
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@28
PS1, Line 28: // Issue 1: store template message w/ or w/o parameters, if just template message then assuming failure is a
: // parameter if something fails once then a second time but this time b/c of a different reason, second log message
: // will be suppressed and we won't know that there's a second cause of this failure
: //
: // Issue 2: Use log.info(msg, arguments) for everything or check for length so Object[] for arguments wont be
: // created before method is invoked. From the Logger Interface regarding this issue:
: // However, this variant incurs the hidden (and relatively small) cost of creating an <code>Object[]</code> before
: // invoking the method, even if this logger is disabled for INFO.
: //
: // Note for Issue 2: Only using INFO level as an example, applies to other Levels too
> These lines look long. Line length needs to be < 100.
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50
PS1, Line 50: static
> As per the current implementation, I don't think this should be static whic
if map isnt static then size would be less of an issue, one map per class essentially
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50
PS1, Line 50: timestamps
> nit: this isn't just the timestamps, so its call-sites read somewhat awkwar
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50
PS1, Line 50: final
> Values are added to this map, so this can't be final.
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@52
PS1, Line 52:
> Nit: Unnecessary space.
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@62
PS1, Line 62: long seconds
> How about using Duration as argument for all variants so that callers don't
if user supplies a unit less than seconds then when it's converted to seconds in this class there will be a rounding error, of course we can use the smallest time unit available (nanoseconds), if anyone else has thoughts please chime in.
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@63
PS1, Line 63: processing(seconds, msg)
> Depending on how pervasive this becomes, rather than mapping the message to
If memory is a concern I can map the hashCode of the String instead
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@119
PS1, Line 119: @param duration number of seconds between each desired log message
> nit: How about calling this something like throttlingIntervalSecs or somesu
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@123
PS1, Line 123: message
> Like other functions, msg should be sufficient.
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@123
PS1, Line 123: processing
> Choose a better name as "processing" doesn't convey much, like shouldLog()?
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@124
PS1, Line 124: current
> nit: maybe "nowSecs"?
Done
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@125
PS1, Line 125: // probably can just use duration instead of gap
> What is is this conversion doing? Isn't 'time' already TimeUnit.SECONDS?
Done
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 18:45:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS5, Line 554: 60L
> With such a reasoning I'd expect it to be logged once in a second then, not
After discussing this offline, it's clear that either 60 second interval, 1 second, or logging once would be simple enough and acceptable :) I don't have a strong opinion here, so feel free to proceed with whatever the decision that in-line with the consensus of the majority of reviewers.
I guess this might be one of those bikeshedding-like things :)
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 18:36:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16400
to look at the new patch set (#5).
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
KUDU-3012: Add a Log Throttler
LogThrottler implements a simple log throttler by logging at most one
message per a time period that can be set at the call-site. Each
instance of this class is designed to throttle regardless of message.
Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
3 files changed, 189 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/16400/5
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@30
PS5, Line 30: ThrottlerLogUtil
nit: LogThrottler
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@34
PS5, Line 34: TODO(mreddy): If logging slows down drastically, explicitly check for arguments length in logging
: * functions and call corresponding function rather than calling function with varargs each time.
: * By calling the varargs function each time, even when it's not needed an Object[] will be created.
This is only really used in tests right now. An alternative would be to remove the Object... arguments altogether, update the test to pass just the string, and update the TODO to efficiently support variadic arguments.
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@43
PS5, Line 43: private long lastLoggedTime = -1;
Do we need to protect concurrent updates to this?
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@43
PS5, Line 43: lastLoggedTime
nit: maybe lastLoggedTimeSecs so usage and units are more obviously correct elsewhere.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Sep 2020 18:46:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16400
to look at the new patch set (#3).
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
KUDU-3012: Add a Log Throttler
LogThrottler uses an instance of Logger and only logs one message
per a time period that can be set at the call-site. Each instance of
this class is designed to throttle regardless of message.
Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
3 files changed, 188 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/16400/3
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Sep 2020 01:42:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS5, Line 554: 60L
> I think we wan't to log it periodically to make sure it's seen. Logs can ro
With such a reasoning I'd expect it to be logged once in a second then, not once in a minute.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 15:16:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Log Throttler
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@63
PS1, Line 63: processing(seconds, msg)
> Performance is also an issue. Hashing a string is way more expensive than h
Among the throttle log implementations I've used in the past, don't recall seeing a separate unique key being supplied.
It's like offloading an implementation detail to the callers and one more thing to think about. As a caller, I just want the message to be throttled.
So that's the argument in favor of not adding another parameter for the callers.
If performance of throttle log becomes an issue then adding separate key would start making sense. Till then I don't see a need for it.
Re: Storing hash code v/s storing string as key.
Java Hashmap implementation will compute the hash code of the string key anyways, so computationally using string v/s hash code as key would be the same. Though storing hashcode would help from memory perspective.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 21:35:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16400
to look at the new patch set (#4).
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
KUDU-3012: Add a Log Throttler
LogThrottler uses an instance of Logger and only logs one message
per a time period that can be set at the call-site. Each instance of
this class is designed to throttle regardless of message.
Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
3 files changed, 188 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/16400/4
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3012: Log Throttler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@63
PS1, Line 63: processing(seconds, msg)
> Among the throttle log implementations I've used in the past, don't recall
Another thought I had was to instead have each ThrottlerLogUtil instance only keep track of a single lastLoggedTimestamp at a time, and circumvent hashing or message/key tracking entirely. The onus is already on the caller to define where they want to throttle in their application. If that's the case, it doesn't seem like much effort for developers to define new ThrottlerLogUtil instances per instance of throttling.
In general, my concern is around throttling messages that are not fixed constants. In such cases, message-based throttling doesn't make sense while also being computationally expensive on account of the frequent hashing.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 22:09:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS5, Line 118: /**
> +1
Done
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@30
PS5, Line 30: LogThrottler wil
> nit: LogThrottler
Done
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@34
PS5, Line 34: TODO(mreddy): Efficiently support variable arguments by explicitly checking for arguments length
: * in logging functions and call corresponding function rather than calling function with varargs
: * every time. This will avoid the hidden cost of creating an Object[] before invoking the method.
> This is only really used in tests right now. An alternative would be to rem
good point, took out the varargs support for now and put it in a TODO
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@43
PS5, Line 43: private long lastLoggedTimeSecs = -1;
> Do we need to protect concurrent updates to this?
Done
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@43
PS5, Line 43: lastLoggedTime
> nit: maybe lastLoggedTimeSecs so usage and units are more obviously correct
Done
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Sep 2020 22:22:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG@9
PS4, Line 9: LogThrottler uses an instance of Logger
> This is part of implementation detail and unnecessary.
Done
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG@11
PS4, Line 11: message.
> message parameters, right?
not sure I understand
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS4, Line 118: final
> We want a single instance per class, so this should be static.
Then multiple instances of AsyncKuduSession will share the same throttler and manipulate the same copy of the lastLoggedTime variable, is that what we want?
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS4, Line 554: 10
> I think 60 secs would be better.
even if that means only log one message per 60 secs?
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@42
PS4, Line 42: private long lastLoggedTime = 0;
> Nit: If time were to start from epoch then the first log message will be th
Done
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@49
PS4, Line 49: * Calls shouldLog method, logs message at trace level if method true, does nothing if false
> Java doc comments are meant for the users and there is no need to mention p
Done
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Sep 2020 00:18:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@42
PS2, Line 42: ThrottlerLogUtil
nit: Maybe name this "LogThrottler" now that it's not a utility class?
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 16:59:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16400
to look at the new patch set (#2).
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
KUDU-3012: Add a Log Throttler
ThrottlerLogUtil uses an instance of Logger and only logs one message
per a time period that can be set at the call-site. Each instance of
this class is designed to throttle regardless of message.
Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
3 files changed, 189 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/16400/2
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50
PS1, Line 50: argume
> if map isnt static then size would be less of an issue, one map per class e
Done
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 01:04:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS5, Line 118: private final LogThrottler throttleClosedLog = new LogThrottler(LOG);
nit: add a comment why this isn't static so it's obvious in the future.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Sep 2020 16:46:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Log Throttler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@63
PS1, Line 63: processing(seconds, msg)
> If memory is a concern I can map the hashCode of the String instead
Performance is also an issue. Hashing a string is way more expensive than hashing an integer type, especially since there is no bound on how long a logged message will be.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 18:54:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@550
PS2, Line 550: ThrottlerLogUtil throttleLog = new ThrottlerLogUtil(LOG);
If we instantiate a new ThrottlerLogUtil every time apply() is called, won't it never throttle anything?
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@29
PS2, Line 29: TODO:
nit: can you suffix this with your id, same below, e.g.
// TODO(awong): foo bar
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@29
PS2, Line 29: If functionality is ever expanded, use ConcurrentHashMap to store multiple messages and
: * the last time it was logged, only one instance of ThrottlerLogUtil will be needed per class then,
: * use integer as key rather than string for performance costs
nit: this can probably be broken into a couple sentences, given you are describing two distinct ideas: hashing based on messages to allow the usage of a single ThrottlerLogUtil instance for multiple call-sites, and hashing integers instead of message for the sake of reducing hashing cost.
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java@46
PS2, Line 46: :
nit: what is the point of this colon?
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java@56
PS2, Line 56:
nit: this seems like too many spaces. Same below.
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 04:49:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS5, Line 118: private final LogThrottler throttleClosedLog = new LogThrottler(LOG);
> nit: add a comment why this isn't static so it's obvious in the future.
+1
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Sep 2020 17:24:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16400/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS3, Line 118: final
> Should this be static?
then multiple instances of AsyncKuduSession will share the same throttler and manipulate the same copy of the lastLoggedTime variable, will leave it as is for now unless anyone has any objections
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/2/java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java@46
PS2, Line 46: :
> Alternatively, you can only throttle 10 messages.
Done
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Sep 2020 21:26:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG@11
PS4, Line 11: gardless
> throttles regardless of message
Done
http://gerrit.cloudera.org:8080/#/c/16400/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16400/5//COMMIT_MSG@7
PS5, Line 7: KUDU-3012: Add a Log Throttler
> As an alternative approach, could it be enough to log that particular warni
decided to go with a log throttler, more flexible if we want to throttler other messages in the future
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118
PS4, Line 118:
> decided to keep it as is for now, if a strong case for making it static is
Done
http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS4, Line 554:
> decided once per minute is fine
Done
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554
PS5, Line 554:
> After discussing this offline, it's clear that either 60 second interval, 1
Ultimately decided to log once every 60 seconds, thanks everyone for all your input
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java:
http://gerrit.cloudera.org:8080/#/c/16400/5/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@34
PS5, Line 34: TODO(mreddy): Efficiently support variable arguments by explicitly checking for arguments length
: * in logging functions and call corresponding function rather than calling function with varargs
: * every time. This will avoid the hidden cost of creating an Object[] before invoking the method.
> The only concern with variadic arguments is performance, right?
that would provide a more flexible implementation, will add functions with 0, 1, 2, or more variable arguments, more details in the Logger interface
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 22:57:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Add a Log Throttler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler
......................................................................
KUDU-3012: Add a Log Throttler
LogThrottler implements a simple log throttler by logging at most one
message per a time period that can be set at the call-site. Each
instance of this class is designed to throttle regardless of message.
Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Reviewed-on: http://gerrit.cloudera.org:8080/16400
Tested-by: Kudu Jenkins
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestLogThrottler.java
3 files changed, 384 insertions(+), 1 deletion(-)
Approvals:
Kudu Jenkins: Verified
Bankim Bhavsar: Looks good to me, approved
Grant Henke: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f
Gerrit-Change-Number: 16400
Gerrit-PatchSet: 8
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>