aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joshua Cohen <jco...@apache.org>
Subject Re: Review Request 54107: changes to intercept and time mybatis invocations
Date Tue, 29 Nov 2016 04:18:39 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54107/#review157197
-----------------------------------------------------------




src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java (line 74)
<https://reviews.apache.org/r/54107/#comment227645>

    fix class name for logger.



src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java (lines
97 - 99)
<https://reviews.apache.org/r/54107/#comment227647>

    Just inline this into the LoadingCache above?



src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java (line 107)
<https://reviews.apache.org/r/54107/#comment227649>

    We generally try and avoid log statements in this type of voice, replace "I'll use metric
name..." with something like, "Using metric name..."
    
    Also, we recently patched all of our log statements to use replacement tokens rather than
string concat (to avoid costly string operations for logs that are not emitted due to logging
levels). Please replace the string concat here with `LOG.warn("... Using metric name '{}'
instead.", INVALID_INVOCATION_METRIC_NAME)`



src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java (line 119)
<https://reviews.apache.org/r/54107/#comment227655>

    What is the performance impact of this interceptor? Do we want to intercept every call?
Should we instead allow for some configurable sampling percentage?



src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java (line
36)
<https://reviews.apache.org/r/54107/#comment227652>

    Can you add some negative test cases as well (e.g. if the first parameter is not an `MappedStatement`)?



src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java (line
46)
<https://reviews.apache.org/r/54107/#comment227651>

    You shouldn't need to create a mock of `FakeClock`, it's already itself essentially a
mock. Instead, the generally usage is along the lines of...
    
        FakeClock clock = new FakeClock();
        
        ...
        
        // advance the clock by 1 second
        clock.advance(Amount.of(1L, Time.SECONDS));
        
        ...
        // Do some more work
        
        // Advance the clock some more...
        clock.advance(Amount.of(10L, Time.SECONDS));
        
        ...
        // Reset the clock...
        clock.setNowMillis(0L);
        
    If you search through the codebase you should be able to find copious examples to crib
from.



src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java (lines
52 - 54)
<https://reviews.apache.org/r/54107/#comment227650>

    `expect(invocation.getArgs()).andReturn(args).times(3)`



src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java (line 52)
<https://reviews.apache.org/r/54107/#comment227653>

    Use `//` style comments, no need for javadoc style here.



src/test/java/org/apache/aurora/scheduler/storage/mem/MemCronJobStoreTest.java (line 42)
<https://reviews.apache.org/r/54107/#comment227654>

    same here.


- Joshua Cohen


On Nov. 29, 2016, 2:36 a.m., Reza Motamedi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54107/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2016, 2:36 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, Mehrdad Nurolahzade, and Santhosh
Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> MyBatis allows us to intercept calls within the execution of a mapped statement. This
allows us to time various mapped statements and ultimately gain more insight on the performance
of the database module.
> 
> This patch introduces an interceptor on MyBatis on `updates` and `query` mapped statements.
I used the following convention to create name for the newly collected stats:
> mybatis.<<the id of the mapped statement>>
> 
> After interception the process is very similar to the one in @Timed-interceptor. SlidingStats
can be used to export interval averages, total milliseconds and the event counts.
> 
> __example stats (from ./vars.json)__
> mybatis.create_tables_events 1
> mybatis.create_tables_events_per_sec 0.0
> mybatis.create_tables_nanos_per_event 0.0
> mybatis.create_tables_nanos_total 592633784
> mybatis.create_tables_nanos_total_per_sec 0.0
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_events 3
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_events_per_sec
0.0
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_nanos_per_event
0.0
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_nanos_total
2858362
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_nanos_total_per_sec
0.0
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_events 333
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_events_per_sec
0.0
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_nanos_per_event
0.0
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_nanos_total 85745680
> mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_nanos_total_per_sec
0.0
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java e7287cec28e7b8ca978c506bfe821f261bc0ac26

>   src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java
PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java
PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 2e560c0d565689703b282391fe49dbf213ee25dc

>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemCronJobStoreTest.java 79999e19454568540c14f91808635bf8dff82bb3

> 
> Diff: https://reviews.apache.org/r/54107/diff/
> 
> 
> Testing
> -------
> 
> Tests are covered in InstrumentingInterceptorTest.
> 
> - testStatIsCreatedOnce
> Tests that each stat is created one time only.
> 
> - testInterceptMarksMetrics
> Tests that invocation is correctly intercepted and then proceeds.
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message