Vojtech Szocs has posted comments on this change.
Change subject: HostGeneralModel: improve "node has upgrade" logic
......................................................................
Patch Set 2: (7 inline comments)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostGeneralModel.java
Line 47: {
Line 48: private static final UIConstants constants =
ConstantsManager.getInstance().getConstants();
Line 49: private static final UIMessages messages =
ConstantsManager.getInstance().getMessages();
Line 50:
Line 51: private static final Logger logger =
Logger.getLogger(HostGeneralModel.class.getName());
I've just realized that logging in UiCommon models happens through ILogger
abstraction provided via Model.getLogger() method, for example:
getLogger().info("message");
Note that GWT UI implements ILogger abstraction (LoggerImpl) using
java.util.logging.Logger so the end result is the same.
To stay consistent with ILogger abstraction, I suggest to remove "logger"
declaration here, and just use getLogger() method as mentioned above.
Line 52:
Line 53: public static EventDefinition RequestEditEventDefinition;
Line 54: private Event privateRequestEditEvent;
Line 55:
Line 674: public boolean shouldAlertUpgrade(ArrayList<RpmVersion> isos,
String hostOs)
Line 675: {
Line 676: boolean alert = false;
Line 677:
Line 678: DateTimeFormat format = DateTimeFormat.getFormat("yyyyMMdd");
//$NON-NLS-1$
I suggest to make "format" static in order to avoid parsing the pattern each
time, for example:
private static final DateTimeFormat dateISOFormat =
DateTimeFormat.getFormat("yyyyMMdd"); //$NON-NLS-1$
After this, you can use "dateISOFormat.parse" instead of "format.parse" in code
below.
Line 679: String nodeInfo[] = hostOs.split("-"); //$NON-NLS-1$
Line 680: String formattedISOVersion = null;
Line 681: for (RpmVersion iso: isos) {
Line 682: try {
Line 676: boolean alert = false;
Line 677:
Line 678: DateTimeFormat format = DateTimeFormat.getFormat("yyyyMMdd");
//$NON-NLS-1$
Line 679: String nodeInfo[] = hostOs.split("-"); //$NON-NLS-1$
Line 680: String formattedISOVersion = null;
Since both "nodeInfo" and "formattedISOVersion" aren't used outside of try
block, I'd suggest to put them inside the try block, for example:
try {
String nodeInfo[] = hostOs.split("-"); //$NON-NLS-1$
String formattedISOVersion = String.valueOf(iso);
...
}
Line 681: for (RpmVersion iso: isos) {
Line 682: try {
Line 683: formattedISOVersion = String.valueOf(iso);
Line 684: // Get last 8 chars which is the date. Example:
2.6.0.20130212
Line 679: String nodeInfo[] = hostOs.split("-"); //$NON-NLS-1$
Line 680: String formattedISOVersion = null;
Line 681: for (RpmVersion iso: isos) {
Line 682: try {
Line 683: formattedISOVersion = String.valueOf(iso);
Instead of String.valueOf() which delegates to RpmVersion.toString() which
returns Version.getValue(), I suggest to use Version.getValue() directly, for
example:
String formattedISOVersion = iso.getValue();
Line 684: // Get last 8 chars which is the date. Example:
2.6.0.20130212
Line 685: formattedISOVersion =
formattedISOVersion.substring(formattedISOVersion.length() - 8);
Line 686: Date dateISO = format.parse(formattedISOVersion);
Line 687: // Split by . nodeInfo[2] and get the first element.
Example: 20120228.fc18
Line 686: Date dateISO = format.parse(formattedISOVersion);
Line 687: // Split by . nodeInfo[2] and get the first element.
Example: 20120228.fc18
Line 688: Date dateNodeISO =
format.parse(nodeInfo[2].split("\\.")[0].trim()); //$NON-NLS-1$
Line 689: if (dateISO.compareTo(dateNodeISO)>0) {
Line 690: alert = true;
Minor comment, I'd rather "return true" here directly.
Line 691: break;
Line 692: }
Line 693: } catch (IllegalArgumentException er) {
Line 694: logger.info("IllegalArgumentException error: " + er);
//$NON-NLS-1$
Line 689: if (dateISO.compareTo(dateNodeISO)>0) {
Line 690: alert = true;
Line 691: break;
Line 692: }
Line 693: } catch (IllegalArgumentException er) {
On bad input, you can also have "string index out of bounds" exceptions and
such, so I'd suggest to catch Exception here, for example:
} catch (Exception ex) {
getLogger().error("Error while trying to determine host upgrade alert", ex);
//$NON-NLS-1$
}
Line 694: logger.info("IllegalArgumentException error: " + er);
//$NON-NLS-1$
Line 695: }
Line 696: }
Line 697: return alert;
Line 693: } catch (IllegalArgumentException er) {
Line 694: logger.info("IllegalArgumentException error: " + er);
//$NON-NLS-1$
Line 695: }
Line 696: }
Line 697: return alert;
Minor comment, I'd rather "return false" here directly and remove "alert"
variable altogether.
Line 698: }
Line 699:
Line 700: private boolean hasManualFenceAlert;
Line 701:
--
To view, visit http://gerrit.ovirt.org/16089
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49c74d3d0d92c74848e0c175095610b6e6b5a88e
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
