Martin Mucha has posted comments on this change. Change subject: core: scanning of configured NICs for health ......................................................................
Patch Set 5: (8 comments) http://gerrit.ovirt.org/#/c/34222/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsNotRespondingTreatmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsNotRespondingTreatmentCommand.java: Line 72: return new FenceExecutor(getVds(), getParameters().getAction()); Line 73: } Line 74: Line 75: private boolean shouldFencingBeSkipped(VDS vds) { Line 76: if (!nicsHealthCheckManager.isSystemHealthy()) { > Please add an alert why fencing was skipped. Done Line 77: return true; Line 78: } Line 79: Line 80: // check if fencing in cluster is enabled http://gerrit.ovirt.org/#/c/34222/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/enginehealth/NicsHealthCheckManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/enginehealth/NicsHealthCheckManager.java: Line 74: log.debug("Performing engine NICs health check on: " + nicsToString(nicsToCheck)); Line 75: Line 76: checkNicsAndUpdateEngineStatus(nicsToCheck); Line 77: Line 78: log.debug("Health check done, engine NICs are " + (engineNicHealthCache.isAllNicHealthy() ? "healthy" : "unhealthy") + "."); > Please use slf4j way of formatting log messages: Done Line 79: } Line 80: Line 81: public boolean isSystemHealthy() { Line 82: return engineNicHealthCache.isAllNicHealthy(); Line 86: for (NetworkInterface nic : nicsToCheck) { Line 87: boolean nicIsHealthy = isNicHealthy(nic); Line 88: boolean updated = engineNicHealthCache.update(nic, nicIsHealthy); Line 89: if (updated && !nicIsHealthy) { Line 90: log.warn(String.format("Engine NIC '%1$s', configured for regular health check, is not UP!", > Please use slf4j way of formatting log messages: Done Line 91: nic.getDisplayName())); Line 92: } Line 93: } Line 94: } Line 99: } catch (SocketException e) { Line 100: String message = String.format("Unable to get network interface '%1$s' status, pronouncing it unhealthy.", Line 101: networkInterface.getDisplayName()); Line 102: log.debug(message, e); Line 103: log.warn(message); > Please use new suggested exception logging: this is new pattern how to log exceptions? I'm aware of (not very common) duplicate logging, warn/error with message, and logging stacktrace with debug. But do we now 'require' that logging stacktraces goes with "Exception" message? question 2: why's this not solved by logger configuration? At least in log4j it should be doable to configure production deployment to throw away stacktraces. It's safer and less error prone ~ i.e. nobody will mistakingly log stacktrace. And on top of that, I cannot see any(pronounce it major or minor) problem with logging stacktraces in production if we're opensource. This way we just lost detailed information at a (marginal) benefit of saving some space. Line 104: return false; Line 105: } Line 106: } Line 107: Line 107: Line 108: /** Line 109: * @return string representation of NetworkInterface list for logging purposes. Line 110: */ Line 111: private String nicsToString(List<NetworkInterface> networkInterfaces) { > Why not use: the reason is in what NetworkInterface.toString produces: "name:em1 (em1)" but you're right. I'd be better to create function which transforms List<NetworkInterface> to List<String> of network interface names and then use stardard function. Line 112: int size = networkInterfaces.size(); Line 113: Line 114: if (size == 0) { Line 115: return ""; Line 141: Line 142: for (String nicName : nicNames) { Line 143: NetworkInterface networkInterface = networkInterfaceByName(nicName); Line 144: if (networkInterface == null) { Line 145: log.warn(String.format("NIC '%1$s' configured to be checked is not found in the system. This NIC will be ignored.", > Please use slf4j way of formatting log messages: Done Line 146: nicName)); Line 147: } else { Line 148: result.add(networkInterface); Line 149: } Line 159: private NetworkInterface networkInterfaceByName(String nicName) { Line 160: try { Line 161: return NetworkInterface.getByName(nicName); Line 162: } catch (SocketException e) { Line 163: log.debug(String.format("Getting NetworkInterface by name '%1$s' failed.", nicName), e); > Please use slf4j way of formatting log messages: Done Line 164: return null; Line 165: } Line 166: } Line 167: Line 223: return result; Line 224: } Line 225: Line 226: private boolean delayOk(Date now, long lastFailure) { Line 227: return now.getTime() - lastFailure > (long) Config.getValue(ConfigValues.EngineNicsHealthCheckDelay); > Why not use: Done Line 228: } Line 229: } -- To view, visit http://gerrit.ovirt.org/34222 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c092ab5d3cf33e1ef83cd0225d2803733fbde89 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
