[
https://issues.apache.org/jira/browse/YUNIKORN-1826?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17735692#comment-17735692
]
Craig Condit commented on YUNIKORN-1826:
----------------------------------------
[~yangpoan] can you do something similar to what YUNIKORN-1825 did for the core
side? I think the initial list looks good. It may be worth looking at how many
Logger() calls exist in each to see if there are opportunities to split or
merge.
I did some very rough counts by Go source file:
{code:java}
$ grep -r 'Logger()' pkg | sed 's/:.*//' | sort | uniq -c
49 pkg/admission/admission_controller.go
8 pkg/admission/conf/am_conf.go
1 pkg/admission/informers.go
4 pkg/admission/metadata/usergroup.go
1 pkg/admission/namespace_cache.go
12 pkg/admission/pki/certs.go
1 pkg/admission/priority_class_cache.go
1 pkg/admission/util.go
47 pkg/admission/webhook_manager.go
9 pkg/appmgmt/appmgmt.go
7 pkg/appmgmt/appmgmt_recovery.go
14 pkg/appmgmt/general/general.go
3 pkg/appmgmt/general/metadata.go
5 pkg/appmgmt/general/podevent_handler.go
7 pkg/appmgmt/sparkoperator/spark.go
3 pkg/cache/amprotocol_mock.go
29 pkg/cache/application.go
5 pkg/cache/application_state.go
70 pkg/cache/context.go
11 pkg/cache/context_recovery.go
2 pkg/cache/context_test.go
16 pkg/cache/external/scheduler_cache.go
11 pkg/cache/node.go
8 pkg/cache/node_coordinator.go
1 pkg/cache/node_state.go
10 pkg/cache/nodes.go
13 pkg/cache/placeholder_manager.go
19 pkg/cache/task.go
3 pkg/cache/task_state.go
15 pkg/callback/scheduler_callback.go
1 pkg/client/apifactory.go
3 pkg/client/apifactory_mock.go
1 pkg/client/clients.go
19 pkg/client/kubeclient.go
6 pkg/client/kubeclient_mock.go
10 pkg/cmd/admissioncontroller/main.go
6 pkg/cmd/shim/main.go
5 pkg/common/resource.go
1 pkg/common/si_helper.go
3 pkg/common/utils/gang_utils.go
11 pkg/common/utils/utils.go
12 pkg/conf/schedulerconf.go
11 pkg/controller/application/app_controller.go
17 pkg/dispatcher/dispatcher.go
5 pkg/log/logger.go
27 pkg/log/logger_test.go
23 pkg/plugin/predicates/predicate_manager.go
1 pkg/plugin/predicates/predicate_manager_test.go
16 pkg/plugin/support/framework_handle.go
14 pkg/schedulerplugin/scheduler_plugin.go
11 pkg/shim/scheduler.go
3 pkg/shim/scheduler_mock_test.go
1 pkg/shim/scheduler_state.go
1 pkg/shim/scheduler_test.go
{code}
I would probably keep the logger names short (i.e. admission.webhookmanager
could simply be admission.webhook, shim.appmanagement.* -> shim.appmgmt.*,
etc.). The shim.cache hierarchy probably needs rethinking a bit. shim.context
might be better for context.go, as it is a fairly large portion of the code.
Additionally, the various *_state.go files are probably best grouped under
shim.fsm (finite state machine). Something like:
* shim.context \{ pkg/cache/context*.go }
* shim.fsm \{ pkg/cache/*_state.go }
* shim.cache.application \{ pkg/cache/application.go }
* shim.cache.node \{ pkg/cache/node*.go }
* shim.cache.task \{ pkg/cache/task*.go }
* shim.cache.external \{ pkg/cache/external/*.go }
* shim.cache.placeholder \{ pkg/cache/placeholder_manager.go }
Some other thoughts:
* Overall, I like using simply shim instead of k8shim as the original API PR
did. Easier to type and remember.
* I think shim.k8sclient could be simplified to shim.client.
* The cmd/* entrypoints (pkg/cmd/shim/main.go and
pkg/cmd/admissioncontroller/main.go) should resolve to shim and admission
respectively.
* The appmgmt plugins should probably each have their own sub-loggers instead
of of being grouped under handlers:
** shim.appmgmt
** shim.appmgmt.general
** shim.appmgmt.sparkoperator
* The plugin packages are a bit of a mess. pkg/plugin/predicates*.go should
probably log to shim.predicates. pkg/plugin/support/framework_handle.go could
log to shim.framework.
* pkg/shim/* should lgo to shim.scheduler, and pkg/schedulerplugin/* to
shim.scheduler.plugin. This reflects more logically how they operate.
* Any test code should either log to the respective logger, or to the "test"
logger.
To do most of the updates for core, I added the new loggers manually to
pkg/log/logger.go (and updated pkg/log/logger_test.go), and then did regex
search / replace in the associated .go files for "log.Logger()" =>
"log.Log(log.LoggerName)" in each group of files. You can probably do something
similar here. Once this is finished, there should be no other usages of
Logger() found, and we can mark that function as Deprecated (see the core PR
for details). We should leave it in place for a while to avoid breaking all the
in-progress PRs that are out there, and circle back before the 1.4 release to
remove it completely.
> Shim: Convert to scoped logging
> -------------------------------
>
> Key: YUNIKORN-1826
> URL: https://issues.apache.org/jira/browse/YUNIKORN-1826
> Project: Apache YuniKorn
> Issue Type: Sub-task
> Components: shim - kubernetes
> Reporter: Craig Condit
> Priority: Major
>
> Identify which specific loggers should be created for the shim. We should
> attempt to provide fine-grained logging, but do so in a way which is most
> useful for users. For example, we should not just blindly set loggers
> per-package, but probably per-subsystem. This also means we should identify
> subsystem boundaries and attempt to make the logger hierarchy match.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
