-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Generate changelog in
|
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.
Broadly looks good, we discussed the listening-executor changes.
I think you'll need changelogs and probably some formatting fixes (you can run ./gradlew spotlessApply
to get the latter in place, usually).
This should give us a good start: I'd say the next step would be to start considering spans within the tasks, though fine with doing that separately/later depending on your capacity.
Map<InetSocketAddress, List<Cell>> hostsAndCells = | ||
HostPartitioner.partitionByHost(clientPool, cells, Cell::getRowName); | ||
Map<InetSocketAddress, List<Cell>> hostsAndCells; | ||
try(CloseableTracer tracer = CloseableTracer.startSpan("partitionByHost")) { |
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.
nit: checkstyle will probably want a space after
Also we should probably include a bit of metadata (the table ref, if it's safe, and cells.size(), maybe)?
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.
Broadly looks good, I think there's one snag around the LoggingArgs
"cells", | ||
String.valueOf(cells.size()), | ||
"tableRef", | ||
Objects.requireNonNull(LoggingArgs.tableRef(tableRef).getValue()), |
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.
Hmm, I was expecting something like safeInternalTableNameOrPlaceholder
actually. If I'm not mistaken that would actually spit out the table's name regardless.
(Internally there's the assumption that table names in this KVS are safe, but still...)
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.
Fixed.
for (Callable<V> task : tasks) { | ||
futures.add(executor.submit(task)); | ||
DetachedSpan detachedSpan = DetachedSpan.start("task"); |
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.
not actionable now: Yep, this is a reasonable first step. Later on we probably want a bit more information about what the tasks are doing (i.e. maybe change the method's signature or also allow users to provide a List where that has metadata about th task), but this would give us some useful signal.
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.
agreed
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.
👍 LGTM
Released 0.340.2 |
Goals (and why):
Improved visibility into kvs-get performance.
Implementation Description (bullets):
Added a few spans to CellLoader#loadWithTs.
Testing (What was existing testing like? What have you done to improve it?):
Concerns (what feedback would you like?):
A bit unsure about changes in TaskRunner
Where should we start reviewing?:
Priority (whenever / two weeks / yesterday):