[
https://issues.apache.org/jira/browse/YARN-9561?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16917140#comment-16917140
]
Eric Badger commented on YARN-9561:
-----------------------------------
Hey [~eyang], in the meantime I got a chance to go back through your patch
comments from a little while ago. Let me know if you have any questions.
bq. In setup_container_paths, it would be good to use fprintf instead of fputs
and include the actual path location. This helps system admin to debug the
error more precisely.
I don't think this should be added in the code that was created by this patch.
The functions that are called from within {{setup_container_paths()}} are the
ones that should give more fine-grained error logging. Because at the
{{setup_container_paths()}} level I have no idea which directory failed to be
created. I would only be able to give a large list. So I think this is an
improvement for the underlying functions, but isn't relevant to this specific
patch.
bq. Can make_string be used instead of strbuf_append_fmt for readability reason
and reduce the need of string format functions? The 16k size seem like a limit
that is easy to reach. make_string may use more memory during string
construction, but maybe it is safer?
The 16k size is just the initial size to create
{{upperdir=%s,workdir=%s,lowerdir=}}. Upperdir and workdir can only be single
directories, so the only way the 16k limit would be exceeded is if these
directory paths were absolutely gigantic. Inside of {{strbuf_append_fmt()}} we
take into account how much memory needs to be allocated and reallocate that
amount. So unless the system itself is out of memory, we will be able to
allocate the full buffer regardless of the number of layers. This would be much
harder to do if we used {{make_string()}} because we would have to concatenate
the strings together or we would end up doing something like what
{{strbuf_append_fmt()}} does anyway.
bq. This part of code doesn't seem to have any effect:
{noformat}
de = is_docker_support_enabled() ? enabled : disabled;
fprintf(stream,
"%11s launch docker container: %2d appid containerid workdir "
"container-script tokens pidfile nm-local-dirs nm-log-dirs "
"docker-command-file resources ", de, LAUNCH_DOCKER_CONTAINER);
{noformat}
It has no effect on the patch related to the OCI code, yes. We fixed up the
display code to be a little bit cleaner and so we modified this to be in line
with the cleaner style.
> Add C changes for the new RuncContainerRuntime
> ----------------------------------------------
>
> Key: YARN-9561
> URL: https://issues.apache.org/jira/browse/YARN-9561
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Eric Badger
> Assignee: Eric Badger
> Priority: Major
> Attachments: YARN-9561.001.patch, YARN-9561.002.patch,
> YARN-9561.003.patch, YARN-9561.004.patch
>
>
> This JIRA will be used to add the C changes to the container-executor native
> binary that are necessary for the new RuncContainerRuntime. There should be
> no changes to existing code paths.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
