-
Notifications
You must be signed in to change notification settings - Fork 265
Refactor SummerBuilder
to make it possible to use Counters
on SummerBuilder
creation
#738
Conversation
There is an issue for that: #711 |
Codecov Report
@@ Coverage Diff @@
## develop #738 +/- ##
===========================================
- Coverage 71.99% 71.86% -0.14%
===========================================
Files 152 153 +1
Lines 3721 3735 +14
Branches 208 202 -6
===========================================
+ Hits 2679 2684 +5
- Misses 1042 1051 +9
Continue to review full report at Codecov.
|
@johnynek I know here is quite a lot of code without test coverage, if you think it worth it to add tests on that - I can do this. But personally I think it's ok to leave this kind of code without tests. |
case class SummerConstructor(get: SummerBuilder) | ||
case class SummerConstructor(get: SummerConstructorSpec) | ||
|
||
trait SummerConstructorSpec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it called SummerConstructorSpec, there's no SummerConstructor in definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After offline discussion renamed it to SummerWithCountersBuilder
.
case class SummerConstructor(get: SummerConstructorSpec) | ||
|
||
trait SummerConstructorSpec { | ||
def builder(counter: Name => Counter with Incrementor): SummerBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, I think we should just change the definition of SummerBuilder to take the counterBuilder as argument and default to a no-op counterBuilder. SummerBuilder is used very rarely and I think it's ok to break it. There are only a couple places that we'll need to fix at Twitter. Seems better than adding a level of indirection here.
@johnynek wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this change in backward compatible manner - it's still possible to create SummerWithCountersBuilder
out of plain SummerBuilder
.
val valueCombinerCrushSize = option(DEFAULT_VALUE_COMBINER_CACHE_SIZE) | ||
val doCompact = option(CompactValues.default) | ||
Summers.async( | ||
cacheSize, flushFrequency, softMemoryFlush, asyncPoolSize, doCompact, valueCombinerCrushSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too long, move each param to separate line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
some minor comments, but it looks like a nice refactor.
case class SummerConstructor(get: SummerWithCountersBuilder) | ||
|
||
trait SummerWithCountersBuilder { | ||
def create(counter: Name => Counter with Incrementor): SummerBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not crazy about with
what about Name => (Counter, Incrementor)
do they have to be bound together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep it this way (or maybe change to just Incrementor
). It should be the same object, not both of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to just Incrementor
.
case class SummerConstructor(get: SummerBuilder) | ||
case class SummerConstructor(get: SummerWithCountersBuilder) | ||
|
||
trait SummerWithCountersBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably want this to extend Serializable
or we may have some corner case issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I prefer to keep it non serializable, this code (creating actual SummerBuilder
with Counter
s) should happen only on submitter node, and SummerBuilder
is an object which should be serializable (and it is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented in code
def apply(builder: StormTopologyBuilder, node: StormNode): SummerBuilder = { | ||
val summerBuilder = builder.get[SummerConstructor](node) | ||
.map { case (_, constructor) => constructor.get }.getOrElse { | ||
logger.info(s"[${builder.getNodeName(node)}] use legacy way of getting summer builder") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we indent here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
@johnynek @pankajroark can you look again did I point all your comments? Thanks! |
👍 |
Currently the way to customize
Summer
instances is not perfect - we have an old 'legacy' way based on a lot of different options and 'new' way with settingSummerBuilder
directly.At the same time with new way it's impossible to fully utilize
Counters
becausejobId
is needed to create them. Also new way is quite verbose.This PR adds a way to use
Counters
inSummerBuilder
and couple ofSummerBuilder
s:Null
,sync
andasync
.