[
https://issues.apache.org/jira/browse/TEZ-3725?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16011212#comment-16011212
]
Jason Lowe commented on TEZ-3725:
---------------------------------
Thanks for the patch!
Doesn't HttpConnection#cleanup already close the underlying stream? Curious
why we need to do it separately. If we do need to do it separately, we will
fail to call cleanup if the stream's close method throws.
For the rejected execution handling, we know we're getting it because the
service was shutdown. I'm skeptical that logging the full exception is going
to be very useful here. Probably sufficient to just log that we ignored the
specified DagDeleteRunnable. (Having a DagDeleteRunnable#toString method to
help log the affected node/dag combination would be useful here.)
Nit:
{code}
AuxiliaryServiceHelper.setServiceDataIntoEnv(
auxiliaryService, ByteBuffer.allocate(4).putInt(0), localEnv);
shufflePort = 0;
{code}
should be:
{code}
shufflePort = 0;
AuxiliaryServiceHelper.setServiceDataIntoEnv(
auxiliaryService, ByteBuffer.allocate(4).putInt(shufflePort),
localEnv);
{code}
so the aux data and local port variable are easier to keep in sync.
The code is still looking up the shuffle service conf for every container
launch context create. All the patch did was move it outside of the lock, but
it's still doing the lookup for every container launch context.
> Cleanup http connections and other unnecessary fields in DAG Deletion tracker
> classes.
> --------------------------------------------------------------------------------------
>
> Key: TEZ-3725
> URL: https://issues.apache.org/jira/browse/TEZ-3725
> Project: Apache Tez
> Issue Type: Sub-task
> Reporter: Kuhu Shukla
> Assignee: Kuhu Shukla
> Attachments: TEZ-3725.001.patch
>
>
> JIRA to provides fixes for the following feedback comments from [~jlowe].
> {code}
> 1. LocalContainerLauncher
> I'm confused why we go through the trouble of serializing the hardcoded value
> of zero into the aux service protocol buffer, stuff it into the env, then
> immediately go fetch it back out and extract the integer from the byte
> buffer. Isn't this a complicated way to say, shufflePort = 0?
> 2. tezDefaultComponentName only needs to be computed when
> cleanupDagDataOnComplete is true. Actually may not be needed at all, see
> related comment for DagDeleteRunnable below.
> 3. DagDeleteRunnable
> tezDefaultComponentName is unused? I think this transitively means pluginName
> is unused in DeletionTracker which would simplify it's constructor signature.
> 4. DeletionTracker
> Nit: addNodeShufflePorts method name being plural implies more than one port
> can be added but it's only for adding a single node, port pair.
> 5. AMContainerHelpers
> As Sidd mentioned before, we should avoid the redundant conf key lookup when
> creating each container launch context.
> 6. DagDeleteRunnable
> Do we need to do any cleanup on the httpConnection?
> 7. DeletionTrackerImpl
> What if the submission to the executor throws RejectedExecutionException
> because the executor was already shutdown and a late dagComplete was invoked?
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
