You are viewing a plain text version of this content. The canonical link for it is here.
Posted to devnull@infra.apache.org by "jihoonson (GitHub)" <gi...@apache.org> on 2018/08/21 23:59:46 UTC
[GitHub] [incubator-druid] jihoonson opened pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Hopefully fixes https://github.com/apache/incubator-druid/issues/6021.
`TaskData.status` and `TaskData.startTime` can be null if the supervisor is stopped gracefully before processing any runNotice which sets them properly.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
Fixed.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Do we really want to kill the task in this case -- I thought it could only happen for a supervisor that is stopping gracefully? Maybe we should just ignore the task, and log a warning, rather than killing it?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on issue #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
@gianm https://github.com/apache/incubator-druid/pull/6207 should fix it.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
Hmm maybe it makes sense to checkpoint because the supervisor should wait for tasks to finish their jobs and they should be able to checkpoint in the middle of indexing.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
Same here. This should never happen. NPE would throw if `taskData.status` is null and this is just a sanity code to see what's null on the potential NPE. I improved the error message.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
This should never happen and even if it occurs, the supervisor would kill the task of corresponding `taskId` and respawn the same task. Please check https://github.com/apache/incubator-druid/pull/6206/files/c46d4681c334709caa5cddbd0ce0c67a7d22eaad#diff-6eee87b3aa4eb3a516965fe6e93e25a4L1138.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on issue #6206: Fix NPE in
KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
@jihoonson Got it, could you please merge master into this branch in order to get that?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
This should never happen. NPE would throw if `taskData.status` is null and this is just a sanity code to see what's null on the potential NPE. I improved the error message.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Is killing the task the right idea here? If we don't know its status is it safe to leave it alone?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Would it be very surprising if this happened? Enough to stop the supervisor run (i.e.: probable bug)?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
I think that we don't want to stop/kill all tasks in the taskGroup just because one of them has processed all assigned events. It could be a checkpoint for an incremental handoff, and we want all tasks to continue running even after the checkpoint. Am I understanding this right?
In other words, it sounds to me like we want to stop/kill all other tasks if any of them has _finished_ (status = success) but we don't want to stop/kill them if it was an incremental handoff.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
I read the code more closely and now I see that the idea is that at `taskDuration`, tasks should do a final publish and exit. So that's what `finalize` is for. The checkpointTaskGroup function, when finalize is true, will check if any task completed, and if so, stop all its replicas. This makes sense, since there is no point in replicas continuing to run if some task in the group is done. (Because they are all doing the same work.)
With your patch, checkpointTaskGroup, when finalize is true, will now kill any task that has null status. I don't see why this is a good thing. After the `taskDuration` is over, we want to trigger a final checkpoint/publish, and then let all tasks in a group keep running until one of them is successful. Killing one with unknown status seems counter-productive to that goal.
Am I wrong -- is there a reason it's a good idea to kill tasks with unknown status in this case?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Similar question here -- when will this get thrown and what will happen when it gets thrown?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
>From the perspective of checkpointing, if I understand this code correctly, the supervisor is checkpointing because one of tasks in a taskGroup has processed all assigned events, so all tasks in the taskGroup can be stopped or killed.
I'm not sure why this code is called when stopping the supervisor though.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
The null `taskData.status` means that the supervisor hasn't updated it yet, so its actual status can be anything. I think this should kill tasks if their status are unknown because this method is supposed to stop all tasks in the given taskGroup.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on issue #6206: Fix NPE in
KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Oh wait, it passed anyway. I guess #6207 isn't required.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Is this comment right? It sounds like it would probably be the other way around (stop gracefully happens before run notices are processed)
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on issue #6206: Fix NPE in
KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Merged master into this branch to get the fixes from #6236. Let's see how this goes.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
I forgot to say one more thing. This code is called only when tasks are running more than `taskDuration`. I don't know what's the idea behind doing checkpoint per taskDuration, but it expects to stop/kill all running tasks. See https://github.com/apache/incubator-druid/blob/master/extensions-core/kafka-indexing-service/src/main/java/io/druid/indexing/kafka/supervisor/KafkaSupervisor.java#L1433.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Thanks.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
Hmm, good point. I thought it makes sense to kill them because [the supervisor is currently killing running tasks if they are not allocated to middleManagers yet](https://github.com/apache/incubator-druid/blob/master/extensions-core/kafka-indexing-service/src/main/java/io/druid/indexing/kafka/supervisor/KafkaSupervisor.java#L1502-L1508).
Maybe it makes more sense to keep them because the unknown task status indicates that the supervisor hasn't updated it yet. I'll fix this.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on issue #6206: Fix NPE in
KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Seeing these, possibly legitimate failures?
```
Failed tests:
KafkaSupervisorTest.testCheckpointForInactiveTaskGroup:2126 java.lang.AssertionError:
Unexpected method call TaskRunner.getRunningTasks():
at org.easymock.internal.MockInvocationHandler.invoke(MockInvocationHandler.java:44)
at org.easymock.internal.ObjectMethodsFilter.invoke(ObjectMethodsFilter.java:94)
at com.sun.proxy.$Proxy43.getRunningTasks(Unknown Source)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor$1.getTaskLocation(KafkaSupervisor.java:312)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor.checkpointTaskGroup(KafkaSupervisor.java:1531)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor.checkTaskDuration(KafkaSupervisor.java:1461)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor.runInternal(KafkaSupervisor.java:898)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor$RunNotice.handle(KafkaSupervisor.java:611)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor$2.run(KafkaSupervisor.java:385)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
expected null, but was:<java.lang.AssertionError:
Unexpected method call TaskRunner.getRunningTasks():
at org.easymock.internal.MockInvocationHandler.invoke(MockInvocationHandler.java:44)
at org.easymock.internal.ObjectMethodsFilter.invoke(ObjectMethodsFilter.java:94)
at com.sun.proxy.$Proxy43.getRunningTasks(Unknown Source)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor$1.getTaskLocation(KafkaSupervisor.java:312)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor.checkpointTaskGroup(KafkaSupervisor.java:1531)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor.checkTaskDuration(KafkaSupervisor.java:1461)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor.runInternal(KafkaSupervisor.java:898)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor$RunNotice.handle(KafkaSupervisor.java:611)
at io.druid.indexing.kafka.supervisor.KafkaSupervisor$2.run(KafkaSupervisor.java:385)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
```
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
Good catch. Fixed.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
> Hmm, good point. I thought it makes sense to kill them because the supervisor is currently killing running tasks if they are not allocated to middleManagers yet.
I think killing unassigned running tasks does make sense, since if a task hasn't even started running yet, it has no hope of catching up so we should just cancel it. However, if there is some risk that the task actually is running but the supervisor just doesn't know where yet, this killing might be over-eager. If that's the case I think it'd be an issue for a separate PR though.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Thanks.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm closed pull request #6206: Fix NPE
in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
[ pull request closed by gianm ]
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Got it, that sounds good.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on issue #6206: Fix NPE in
KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
I checked out the TeamCity failures, they are all fixed in #6236 and are not related to this patch. The Travis failure looked spurious so I retried it.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] jihoonson commented on pull request #6206:
Fix NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "jihoonson (GitHub)" <gi...@apache.org>.
Probably we shouldn't checkpoint while stopping the supervisor, and this would be a different issue.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
Got it, thanks.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on pull request #6206: Fix
NPE in KafkaSupervisor.checkpointTaskGroup
Posted by "gianm (GitHub)" <gi...@apache.org>.
When will this get thrown and what will happen when it gets thrown? I'm wondering what the user experience is going to be like.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6206 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org