aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Reza Motamedi <reza.motam...@gmail.com>
Subject Re: Review Request 54107: changes to intercept and time mybatis invocations
Date Wed, 30 Nov 2016 06:51:46 GMT


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java,
line 46
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571611#file1571611line46>
> >
> >     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.

I am not sure if FakeClokc let me do what I want here. I want the clock to advance inside
the interceptors execution, i.e., first time nowmillis called give me this, next time give
me that. I aggree that I should just mock a Clock, not a FakeClock.


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java,
line 74
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571610#file1571610line74>
> >
> >     fix class name for logger.

Done.


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java,
line 107
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571610#file1571610line107>
> >
> >     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)`

Makes sense.


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java,
line 119
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571610#file1571610line119>
> >
> >     What is the performance impact of this interceptor? Do we want to intercept
every call? Should we instead allow for some configurable sampling percentage?

Not sure if we can sample without intercepting. Once intercepted what is done is only two
calls to Clock::nowNanos and a subtraction. I really don't think that it is an expensive operation.


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java,
lines 52-54
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571611#file1571611line52>
> >
> >     `expect(invocation.getArgs()).andReturn(args).times(3)`

I didn't know I can do that. Cool!


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java, line
52
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571612#file1571612line52>
> >
> >     Use `//` style comments, no need for javadoc style here.

done.


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/mem/MemCronJobStoreTest.java,
line 42
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571613#file1571613line42>
> >
> >     same here.

ditto.


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java,
lines 97-99
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571610#file1571610line97>
> >
> >     Just inline this into the LoadingCache above?

Done.


> On Nov. 29, 2016, 4:18 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java,
line 36
> > <https://reviews.apache.org/r/54107/diff/3/?file=1571611#file1571611line36>
> >
> >     Can you add some negative test cases as well (e.g. if the first parameter is
not an `MappedStatement`)?

I added an additional test for this. Not so sure if that is what you had in mind. Please check.


- Reza


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


On Nov. 30, 2016, 6:14 a.m., Reza Motamedi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54107/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2016, 6:14 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