You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/07/30 07:31:27 UTC
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4228: Dont add host back after agent service restart
DaanHoogland commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r462482006
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
throw e;
} catch (Exception e) {
String msg = " can't setup agent, due to " + e.toString() + " - " + e.getMessage();
s_logger.warn(msg);
} finally {
+ // set guid to null once host is added
Review comment:
is this comment correct? initialGuid might have a value other than null.
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
Review comment:
this is not really saying it. what did we fail at for what reason with (and this part is there) what message.
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
throw e;
} catch (Exception e) {
String msg = " can't setup agent, due to " + e.toString() + " - " + e.getMessage();
s_logger.warn(msg);
} finally {
+ // set guid to null once host is added
Review comment:
exactly. it is not necessarily null. I think you'd better remove this comment, it is confusing.
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
Review comment:
ok, than let's at least make the text describing our course of action:
```suggestion
s_logger.error("DiscoveredWithErrorException caught and rethrowing, message; " + e.getMessage());
```
----------------------------------------------------------------
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:
users@infra.apache.org