paul-rogers commented on code in PR #13790:
URL: https://github.com/apache/druid/pull/13790#discussion_r1103876331
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -331,8 +331,12 @@ public Optional<MSQErrorReport> runTask(final Closer
closer) throws Exception
final StageDefinition stageDefinition = kernel.getStageDefinition();
if (kernel.getPhase() == WorkerStagePhase.NEW) {
- log.debug("New work order: %s",
context.jsonMapper().writeValueAsString(kernel.getWorkOrder()));
+ if (log.isDebugEnabled()) {
+ log.debug("Processing work order: %s",
context.jsonMapper().writeValueAsString(kernel.getWorkOrder()));
+ } else {
+ log.info("Processing work order for stage[%d]",
kernel.getStageDefinition().getStageNumber());
Review Comment:
Would recommend first emitting the info-level general comment. If debug is
enabled, emit the work order as well. Else when searching for this message, one
has to know what log level was set to know which message to search for.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -520,6 +525,7 @@ public InputStream readChannel(
@Override
public void postWorkOrder(final WorkOrder workOrder)
{
+ log.info("Got work order for stage[%d]", workOrder.getStageNumber());
Review Comment:
Nit: better to use standard English spacing: a space between "stage" and the
bracket: `stage [%d]`.
The brackets are really only needed for items that can contain spaces, so we
know what is the message and what is the value. But, this is a number. so
`state %d`.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -603,6 +611,11 @@ public ClusterByStatisticsSnapshot
fetchStatisticsSnapshot(StageId stageId)
@Override
public ClusterByStatisticsSnapshot
fetchStatisticsSnapshotForTimeChunk(StageId stageId, long timeChunk)
{
+ log.debug(
+ "Fetching statistics for stage:[%d] with timechunk:[%d]",
Review Comment:
Edit as above `stage %d, time chunk %d`
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -444,6 +448,7 @@ public Optional<MSQErrorReport> runTask(final Closer
closer) throws Exception
@Override
public void stopGracefully()
{
+ log.info("Stopping gracefully");
Review Comment:
Does the logger emit the task ID? If not, it might help to say what is
stopping gracefully.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -582,12 +588,14 @@ public void postCleanupStage(final StageId stageId)
@Override
public void postFinish()
{
+ log.info("Finish received");
kernelManipulationQueue.add(KernelHolder::setDone);
}
@Override
public ClusterByStatisticsSnapshot fetchStatisticsSnapshot(StageId stageId)
{
+ log.info("Fetching statistics for stage:[%d]", stageId.getStageNumber());
Review Comment:
As above. Inconsistent use of colon. No colon is needed here: `stage %d`.
Does the logger automagically fill in the task ID? If not, would be good to
include that so we can find events for a specific task.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/kernel/worker/WorkerStageKernel.java:
##########
@@ -210,6 +212,12 @@ private void assertPreshuffleStatisticsNeeded()
private void transitionTo(final WorkerStagePhase newPhase)
{
if (newPhase.canTransitionFrom(phase)) {
+ log.info(
Review Comment:
Same comments as above.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -582,12 +588,14 @@ public void postCleanupStage(final StageId stageId)
@Override
public void postFinish()
{
+ log.info("Finish received");
Review Comment:
Finish received for what 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
