weizhouapache commented on code in PR #6892:
URL: https://github.com/apache/cloudstack/pull/6892#discussion_r1073789681
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHABase.java:
##########
@@ -33,7 +35,7 @@ public class KVMHABase {
private long _timeout = 60000; /* 1 minutes */
protected static String s_heartBeatPath;
protected long _heartBeatUpdateTimeout = 60000;
- protected long _heartBeatUpdateFreq = 60000;
+ protected long _heartBeatUpdateFreq =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_HEARTBEAT_UPDATE_FREQUENCY);
Review Comment:
@GutoVeronezi
I think I have clearly expressed my opinion. I have no objection if you want
only `kvm.heartbeat.update.frequency` to be configurable, it is ok to me. I
also noticed that there are some other related properties can be configurable
as well. Do you need me to point them out ? I believe you have noticed them too.
This might happen in the future: the heartbeat update timeout (60seconds) or
a setting else is not good, then externalize it to agent.properties. For your
company, you need to develop, review, test, release and deploy it again. The
community needs to review and test again. It is good if you have discussed it.
Hope it will not come true.
You can of course submit your PRs as individual contributors. The fact we
must face is, you/we are also part of a company, most of our changes are based
on the company's requirements. The rule of merging PR is, PR can only be merged
with 2 approvals (including 1 verification unless it is not required) and smoke
tests pass. If you look at the PRs merged recently, most have been verified by
@DaanHoogland. However, the community does not have resources to manually test
every PR ( I know some PRs have been merged without manual verification, which
is not good). I personally encourage the companies which have good resources to
test their PRs and share the results, so both the company and the community can
benifit from their contribution (including not only creating PRs, but also
discussing/reviewing/testing PRs).
Back to the topic, @stephankruggg can you explain a bit why you made this
setting configurable, but not for other settings ?
--
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]
