JoaoJandre commented on PR #7131:
URL: https://github.com/apache/cloudstack/pull/7131#issuecomment-1491900877
> We need to validate packaging and smoketests across all hypervisor (and
permutations on zone types, storage etc), was there any done? Does changing
logging library has side-effects for existing codebase?
We have been doing packaging and smoke tests for a while here in this PR. We
are working hard to find the issues and fix them as quickly as possible.
> I think we would also need to do upgrade tests for all supported
hypervisors. Have we done any testing on this, could you share all such details
of testing you may have done?
I tested this change in KVM and VMware with cluster and zone-wide storage,
Veeam integration, Kubernets, and many other features of ACS. I hope this PR is
merged soon; then, we will have plenty of time for the community to test other
supported hypervisors and features/workflows that we have not covered.
> I see some 120+ PRs, would merging this cause conflicts on them? Is there
a way to minimise conflicts and side-effects on rest of the community PRs?
Yes, this will cause a lot of conflicts, the first commit in this PR
normalizes the loggers, in an attempt to minimize the number of places touched
by the following commits. Therefore, the conflicts tend to be quite easy to
address, as we will only see conflicts in lines that are logging something.
Furthermore, in the future, if we have to make another such change to the
logging framework we use, the normalization proposed in this PR will minimize
the work to be done.
> Do we need an advisory for users, on how this would affect their current
logging config, affect their plugins and any other integrations? (a lot of
plugins aren't in the upstream repo but managed/shipped by users in the
community)
Yes, the new configuration file format is not compatible with the old one, I
just added a notice about that in the PR description. When the release with
this change is due, we will need to add that note to the release notes.
> If we're considering this, should we also look at upgrading related
dependencies such as gson?
Through the development and testing of this PR, I did not see any need to
upgrade other dependencies, but if it is the case, we can discuss that upgrade
in another PR.
> Speculating here - if only the parent classes has logger, has anybody
tested if logging by a child-class prints the right class name in the logs (for
example, [c.c.c.ClusterManagerImpl] in the log file)?
This works because of a object-oriented programming concept called
inheritance, here is an example:
```
2023-03-30T12:39:17,688 DEBUG [c.c.h.XenServerGuru]
(StatsCollector-2:[ctx-e7a7ea68]) (logid:c2943f60) We are returning the default
host to execute commands because the command is not of Copy type.
2023-03-30T12:39:17,773 DEBUG [c.c.a.t.Request]
(StatsCollector-2:[ctx-e7a7ea68]) (logid:c2943f60) Seq 31-5795569770472407063:
Received: { Ans: , MgmtId: 90520745551922, via: 31(cloudstack-lab-host-3),
Ver: v1, Flags: 10, { GetStorageStatsAnswer } }
2023-03-30T12:39:17,799 DEBUG [c.c.h.o.r.Ovm3HypervisorGuru]
(StatsCollector-2:[ctx-e7a7ea68]) (logid:c2943f60) getCommandHostDelegation:
class com.cloud.agent.api.GetStorageStatsCommand
```
Neither XenServerGuru nor Ovm3HypervisorGuru has the logger explicitly
declared, but both of them extend the ComponentLifecycleBase, which has a
logger defined as `protected Logger logger =
LogManager.getLogger(getClass());`. When any of the child classes are
instantiated, the getClass() method will return themselves (the children's
classes) and not the parent class; thus, their logger will be instantiated
correctly.
--
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]
