[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-589948906
 
 
   > I think the patch looks in a decent shape after the refactor.. There are 
some minor comments I posted...
   > 
   > Also, few things you mentioned you'd do in follow-on tasks..
   > 
   > * too-large responses
   > * metrics support
   > * thrift-api equivalent
   > 
   > Please track them as sub-tasks so that we don't lose track of them...
   
   No worries, I am full aware of all follow up tasks and going to create JIRAs 
right after merging this.
   Thanks


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-586820503
 
 
   @ndimiduk if you have time today, please review


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-586575972
 
 
   Using thread-safe wrapper over EvictingQueue makes life simpler, however it 
does not have any perf improvement because earlier I was using lock to write 
data in array in thread safe manner. However, using it has good advantages over 
not losing any writes specifically if manual locks time out or get 
InterruptedException, and of course less manual code to maintain. Thanks 
@bharathv for the suggestion.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-586385701
 
 
   Failed tests in recent build are passing locally, not relevant to the PR 
changes


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-586127846
 
 
   > No, I do not thing it's good to maintain uniform print format across all 
client. Anything that is providing display functionality for an object will 
need control over how the content is formatted.
   > 
   > I maintain that pretty-printed json does not belong in the object's 
`toString` method. The abundantly common use-case for `toString` is the logging 
infrastructure, which has different requirements from the shell's user 
interface. Please just use the Apache Commons Lang `ToStringBuilder` (I prefer 
it's `SHORT_PREFIX_STYLE`) for that purpose.
   > 
   > If you would like the object to control it's own formatting as a 
convenience to a variety of clients (something I find dubious but why not), I 
have no problem with putting this concern behind a separate method in 
`SlowLogRecord`, such as `toFormattedJson`. My main objection to this approach 
is that this introduces a dependency into `SlowLogRecord`, what is otherwise a 
simple POJO, onto an unrelated provider of json representation.
   > 
   > Also, you may not have noticed that the shell has it's own class for 
handling display, called `Formatter`, under 
`hbase-shell/src/main/ruby/shell/formatter.rb`. I haven't used it myself, but 
it seems purpose-built to handle the use-case you have.
   
   Thanks @ndimiduk 
   I agree with your comment and updated the PR with recent commit.
   I have addressed all your concerns so far. Please review.
   Thank you


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-585829397
 
 
   > Thanks for addressing all my concerns so far @virajjasani . I've tried to 
resolve all the inline discussions. Here's a summary of what I think remains 
outstanding.
   > 
   > * log large responses (follow-on task)?
   >   
   >   * don’t assume “tooLarge” and “tooSlow” are mutually exclusive.
   > * replicate log entries into a file on HDFS or a new system table 
(follow-on task)?
   > * expose slow/large responses as a metric (follow-on task)?
   > * do we need to ship with thrift api parity?
   > * push filter and limit logic into the server, so all clients have the 
same experience
   >   
   >   * I think this requires adding a `TooSlowRequest` protobuf message with 
filter and limit parameters
   >   * Maybe then `SlowLogPayload` is renamed to `SlowLogRecord` or something 
like that
   > * need separation of concerns between SlowLog.toString and formatting of 
output in the shell
   > * avoid adding to the surface area of HConstants
   > * proper interrupt handling within SlowLogEventHandler
   > * update configuration adoc to reference the feature adoc
   > * test cleanup
   >   
   >   * avoid sleeping for fixed time units, instead use waitFor
   >   * maybe combine some methods containing repeated logic, introduce a 
helper method or two
   
   Thanks @ndimiduk for all your review. I have tried to cover these points in 
my recent commit except for `need separation of concerns between 
SlowLog.toString and formatting of output in the shell`. The reason being shell 
is not able to use Gson as efficiently as keeping `Gson.toJson()` in toString 
of SlowLogRecord because we have some serialization filters (neither does shell 
have any good pretty print library that works with our POJO - AFAIK). shell or 
any client would receive `List` as output of 
`Admin.getSlowLogResponses()`. In a way, keeping `Gson.toJson()` within 
`toString()` would ensure all clients can print output in pretty print format. 
Do you think this is good to maintain uniform print format across all clients?
   
   I have covered other points provided by you, please review.
   Thanks
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-584788210
 
 
   > > > 1. Since the detailed info is going through this ring buffer 
subsystem, why do we continue to log at all? Why not replace the logging with a 
metric?
   > > 
   > > 
   > > 
   > > 1. Ring Buffer is not a replacement of slow logs that we write to file, 
it's just a complete representation of slow logs, whereas in log files we might 
have trimmed logs. We also have plan to write all slowlogs to HDFS (optionally) 
and provide HDFS file path to user optionally, at that time we can think of 
removing logging in log files. However, I am not sure about metric. Which 
metric are you suggesting to expose? You mean slow logs with some parameters?
   > 
   > I'm suggesting we do away with file logging and instead expose the 
presence of slow activity as a (new?) metric.
   > 
   > By the time this patch lands, we'll have the logged information in two 
places, neither of which, on their own, allow an operator to easily see how 
prevalent the problem is. I'm suggesting that we could instead provide a metric 
that says "this region server is experiencing N slow request per time interval 
T", as well as providing detailed incident fidelity via this new mechanism.
   
   Thanks @ndimiduk 
   Would you be fine with exposing metric as a sub task?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-582723753
 
 
   Please review @ndimiduk 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-581747580
 
 
   @saintstack I will start creating necessary sub-tasks to this. For now, 
having ring buffer might be a good addition to 2.3.0 release for debugging slow 
RPC calls.
   Please review


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-579605235
 
 
   > I think you misunderstood me here...
   > 
   > "With the patch we would run at RS entrance only and for any slow running 
query it will go to this ring buffer running at RS. Just that this patch 
doesn't include storing slow logs in HDFS, which is also a good idea and 
probably up for a subtask given the priority is storing slow logs in ring 
buffer first. Storing them in HDFS would be just an option as part of another 
Jira(sub-task)."
   > 
   > What I was saying is that your ringbuffer only keeps dfsclient ops. I was 
wondering if we could put actual requests into a system like this. If they are 
taking a long time, we could inspect the ringbuffer to see which type of 
queries are taking a while. It would be coarser grained than the dfs accesses 
only. Was just wondering if you think we could do this in a follow-on based on 
work h ere.
   
   Oh okk, my bad. Looks good for the follow-up task for sure, unless we are 
already putting these requests in ring buffer. One question: Does actual 
request not come to `RpcServer.logResponse()` already? If not, then definitely 
+1 for follow-up task. 
   I thought all requests already do land in RpcServer. No?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-579541178
 
 
   @ndimiduk @saintstack Would you be able to spend some time for review 
today?(I am available today) If not, I can work on this after 2/3 days.
   Thanks


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-579162619
 
 
   > A couple high-level questions:
   > 
   > 1. Since the detailed info is going through this ring buffer subsystem, 
why do we continue to log at all? Why not replace the logging with a metric?
   > 2. Should entries of the ring buffer expire after a period of time?
   
   1. Ring Buffer is not a replacement of slow logs that we write to file, it's 
just a complete representation of slow logs, but in logging we might have 
trimmed logs. We also have plan to write all slowlogs to HDFS (optionally) and 
provide HDFS file path to user optionally, at that time we can think of 
removing logging in application. However, I am not sure about metric? Which 
metric are you suggesting to expose?
   2. IMHO if we are going to achieve significant resource utilization with TTL 
of ring buffer entries, we can have a sub-task to this Jira, but do we really 
need it? I feel it's fine to let them stay in the buffer. Anyways size of the 
buffer and whether to even use ring buffer is up-to user. However, if required, 
I am fine with a sub-task.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-579052854
 
 
   > I sent a note to dev@ giving a status update. If you don’t find there what 
you need, or have some comments, please reply on that thread.
   > […](#)
   > On Mon, Jan 27, 2020 at 16:42 Viraj Jasani ***@***.***> wrote: Thanks for 
the review @ndimiduk  I will try to be on top of 
this. Could you please let me know if you have any estimate of 2.3.0 release? 
May be by end of next week? — You are receiving this because you were 
mentioned. Reply to this email directly, view it on GitHub 
<#754?email_source=notifications_token=AAALDLBPT5FPVY6OIEYVSRLQ755PDA5CNFSM4JEQSSC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKBTWZQ#issuecomment-579025766>,
 or unsubscribe 

 .
   
   Sure @ndimiduk , I got your dev@ message immediately after asking the 
question here :)
   Thanks


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-579025766
 
 
   Thanks for the review @ndimiduk 
   I will try to be on top of this. Could you please let me know if you have 
any estimate of 2.3.0 release? May be by end of next week?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-577086481
 
 
   Pushed new commit, rebased, we have new QA results available too. Please 
take a look @saintstack 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-576961336
 
 
   > Skimmed.
   > 
   > One thought was that we'd want to run a buffer at the RS entrance too, not 
just at the point where we interact with HDFS. This patch would not preclude us 
doing such a thing?
   
   With the patch we would run at RS entrance only and for any slow running 
query it will go to this ring buffer running at RS. Just that this patch 
doesn't include storing slow logs in HDFS, which is also a good idea and 
probably up for a subtask given the priority is storing slow logs in ring 
buffer first. Storing them in HDFS would be just an option as part of another 
Jira(sub-task).


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-565830334
 
 
   Please review @xcangCRM 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-564146946
 
 
   Please review @ramkrish86 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-559696527
 
 
   @guangxuCheng Could you please review?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-557908675
 
 
   Please review @anoopsjohn @saintstack 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-555150670
 
 
   Please review @apurtell @busbey @guangxuCheng 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] virajjasani commented on issue #754: HBASE-22978 : Online slow response log

[GitBox]

virajjasani commented on issue #754: HBASE-22978 : Online slow response log
URL: https://github.com/apache/hbase/pull/754#issuecomment-546956454
 
 
   Requesting your reviews @apurtell @busbey 


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:
us...@infra.apache.org


With regards,
Apache Git Services