rhtyd commented on pull request #4966:
URL: https://github.com/apache/cloudstack/pull/4966#issuecomment-838039333
@DaanHoogland I've just shared my preference that is around optimising
around reviewers' and lab resources time. One can get the same thing with a
single PR with individual small commits that are easier to review (Github
allows you to review each commit individually). The benefit of the latter is
you save a lot of overhead in people's time in tracking/test, and on smoketest
resources.
@GutoVeronezi what you've raised is a valid issue, however, there was no
discussion around a new standard/consistent format for people to follow for new
code. For example and as a suggestion, instead of `NicProfile {"id": 1, "vmId":
2, "reservationId": "3", "iPv4Address": "192.168.0.1", "broadcastUri": "..."}`,
something like `NicProfile [id=1, vmId=2, reservationId=3,
iPv4Address=192.168.0.1, ...]` may read better, and developers don't need to
work with handwritten json. This is merely an example - I'm not asking you to
change the string format.
Here something to think about:
- You can present the enhancement cleanups under a standard initiative,
start discussion on dev@, i.e. what should be a new standard on improving logs,
what new developers, new feature/changes should follow. For example, Gabriel's
suggestion on a standard logging pattern is great feedback.
- A central utility/method that can help switch between formats of objects
(json, csv, ...), if that is even feasible? Do a poc with your proposal.
- Should we log internal DB IDs in logs? Is the cleanup PRs is replacing old
bad logging style in a new way?
- I've seen customers/users who pass logs through an aggregation system such
as Splunk where they may have rules to make sense of things. Many old dinosaurs
in the community like me are used to a pattern/style of reading logs and code
i.e. have we considered what could/would break for existing users and
developers, or their approach as their eyes are set looking for something
they'll not adjust (written with humour - this point can be completely ignored
we don't maintain backward compatibility for logs)
On the issue of context, frankly, any PR that is more than a 100 lines of
changes is not going to get a thorough review for each and every line by
2xreviewers, it's simply not feasible at least for me and people I know.
Instead, we use smoketests and other means (manual QA, design/architecture
review, consistency/pattern of changes, ...) to validate such PRs. I would club
all such PRs around the same initiative which in this particular case is around
logging/refactoring improvements. Splitting the work around the same initiative
in 10 different PRs camouflaging under different `contexts` will just make the
process a lot slower and you're essentially in some way telling the community
(of dinosaurs :D) that you want them to work hard in their "free" time and you
essentially don't care of the overhead it causes them on
reviewing/tracking/testing/...
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
