Omer Frenkel has posted comments on this change. Change subject: core: remove ovirt logger from vdsbroker and friends ......................................................................
Patch Set 3: (10 comments) http://gerrit.ovirt.org/#/c/34374/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetConfigurationValueQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetConfigurationValueQuery.java: Line 25: } Line 26: returnValue = Config.<Object> getValue(value, version); Line 27: } catch (Exception e) { Line 28: log.error("Unable to return config parameter {}: {}", getParameters(), e.getMessage()); Line 29: log.debug("Exception", e); > logs should be free of stack trace as much as possible, unless there are re ok i agree here we can skip logging the whole exception Line 30: } Line 31: } Line 32: Line 33: getQueryReturnValue().setReturnValue(returnValue); http://gerrit.ovirt.org/#/c/34374/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SearchQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SearchQuery.java: Line 413: log.error("Sql Injection in search: {}", getParameters().getSearchPattern()); Line 414: data = null; Line 415: } catch (RuntimeException ex) { Line 416: log.warn("Illegal search: {}: {}", getParameters().getSearchPattern(), ex.getMessage()); Line 417: log.debug("Exception", ex); > also here ok let leave it in debug Line 418: data = null; Line 419: } Line 420: return data; Line 421: } http://gerrit.ovirt.org/#/c/34374/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/CreateVmVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/CreateVmVDSCommand.java: Line 60: ResourceManager.getInstance().RemoveAsyncRunningVm(getParameters().getVmId()); Line 61: } Line 62: } catch (Exception e) { Line 63: log.error("Error in excuting CreateVmVDSCommand: {}", e.getMessage()); Line 64: log.debug("Exception", e); > also here please log exception as error Line 65: if (command != null && !command.getVDSReturnValue().getSucceeded()) { Line 66: ResourceManager.getInstance().RemoveAsyncRunningVm(getParameters().getVmId()); Line 67: } Line 68: throw new RuntimeException(e); Line 96: Guid guid = getParameters().getVm().getId(); Line 97: String vmName = getParameters().getVm().getName(); Line 98: VmDynamic vmDynamicFromDb = DbFacade.getInstance().getVmDynamicDao().get(guid); Line 99: if (ResourceManager.getInstance().IsVmDuringInitiating(getParameters().getVm().getId())) { Line 100: log.info("Vm Running failed - vm {}:{} already running", guid, vmName); > because the user cares about the name and the developer cares about the gui thats ok, so please switch above as well to be name and then guid, so it will match the others Line 101: getVDSReturnValue().setReturnValue(vmDynamicFromDb.getStatus()); Line 102: return false; Line 103: } else { Line 104: VMStatus vmStatus = vmDynamicFromDb.getStatus(); http://gerrit.ovirt.org/#/c/34374/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java: Line 382: } Line 383: } catch (Exception e) { Line 384: if (e.getCause() != null) { Line 385: log.error("CreateCommand failed: {}", e.getCause().getMessage()); Line 386: log.debug("Exception", e); > same here and the 3 below please log exceptions as errors here and 3 below as well Line 387: throw new RuntimeException(e.getCause().getMessage(), e.getCause()); Line 388: } Line 389: log.error("CreateCommand failed: {}", e.getMessage()); Line 390: log.debug("Exception", e); http://gerrit.ovirt.org/#/c/34374/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java: Line 264: "Failed to refresh VDS, continuing, vds ='{}' ('{}'): {}", Line 265: vds.getName(), Line 266: vds.getId(), Line 267: ex.getMessage()); Line 268: log.debug("Exception", ex); please log exception as error Line 269: } Line 270: Line 271: private static void logException(final RuntimeException ex) { Line 272: log.error("ResourceManager::refreshVdsRunTimeInfo", ex); http://gerrit.ovirt.org/#/c/34374/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java: Line 226: new SetVdsStatusVDSCommandParameters(_vds.getId(), VDSStatus.Error)); Line 227: } Line 228: } catch (Throwable t) { Line 229: log.error("Failure to refresh Vds runtime info: {}", t.getMessage()); Line 230: log.debug("Exception", t); i think its better to keep this in error Line 231: throw t; Line 232: } Line 233: moveVDSToMaintenanceIfNeeded(); Line 234: } Line 1273: logicalName = getDeviceLogicalName((Map<?, ?>) vm.get(VdsProperties.GuestDiskMapping), deviceId); Line 1274: } catch (Exception e) { Line 1275: log.error("error while getting device name when processing, vm '{}', device info '{}' with exception, skipping: {}", Line 1276: vmId, device, e.getMessage()); Line 1277: log.debug("Exception", e); please log exception as error Line 1278: } Line 1279: } Line 1280: Line 1281: if (deviceId == null || vmDevice == null) { http://gerrit.ovirt.org/#/c/34374/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FutureVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FutureVDSCommand.java: Line 75: httpTask.cancel(true); Line 76: VDSNetworkException ex = new VDSNetworkException("Timeout during xml-rpc call"); Line 77: ex.setVdsError(new VDSError(VdcBllErrors.VDS_NETWORK_ERROR, "Timeout during xml-rpc call")); Line 78: setVdsRuntimeError(ex); Line 79: log.error("Timeout waiting for VDSM response. " + e); actually, here we might be good with exception message only in error, and stack trace in debug, since we know its timeout/network. roy? Line 80: throw e; Line 81: } catch (Exception e) { Line 82: log.error("Error: {}", e.getMessage()); Line 83: log.debug("Exception", e); Line 79: log.error("Timeout waiting for VDSM response. " + e); Line 80: throw e; Line 81: } catch (Exception e) { Line 82: log.error("Error: {}", e.getMessage()); Line 83: log.debug("Exception", e); please use error Line 84: setVdsRuntimeError(e instanceof RuntimeException ? (RuntimeException) e : new RuntimeException(e)); Line 85: } Line 86: return getVDSReturnValue(); Line 87: } -- To view, visit http://gerrit.ovirt.org/34374 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae367e2c2fe3df862c8a345ca73575198f4c6edd Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
