You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@cloudstack.apache.org by bvbharatk <gi...@git.apache.org> on 2015/09/16 08:37:52 UTC

[GitHub] cloudstack pull request: CLOUDSTACK-8855 Improve Error Message for...

GitHub user bvbharatk opened a pull request:

    https://github.com/apache/cloudstack/pull/837

    CLOUDSTACK-8855 Improve Error Message for Host Alert State and reconnect host API.

    earlier we were eating up exceptions from the lower layer which resulted in improper error messages. fixed partially by throwing exceptions and catching them at the appropriate layer.
    
    This also fixes the host alters. Earlier in the host alerts window we are listing only the alters, this dose not tell why the host when to alert state. This is fixed by adding the alert message to the host alerts.
    (changes in AlertManagerImpl.java). 
    
    tested manually.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/bvbharatk/cloudstack CLOUDSTACK-8855

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/cloudstack/pull/837.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #837
    
----
commit 7920ecfbbb92336bafd0804d1db244d40b9c4852
Author: Bharat Kumar <bh...@citrix.com>
Date:   2015-04-07T06:15:13Z

    CLOUDSTACK-8855 Improve Error Message for Host Alert State

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #837: CLOUDSTACK-8855 Improve Error Message for Host...

Posted by rafaelweingartner <gi...@git.apache.org>.

Github user rafaelweingartner commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/837#discussion_r106185108
  
    --- Diff: api/src/org/apache/cloudstack/api/command/admin/host/ReconnectHostCmd.java ---
    @@ -100,17 +103,18 @@ public Long getInstanceId() {
         @Override
         public void execute() {
             try {
    -            Host result = _resourceService.reconnectHost(this);
    -            if (result != null) {
    -                HostResponse response = _responseGenerator.createHostResponse(result);
    -                response.setResponseName(getCommandName());
    -                this.setResponseObject(response);
    -            } else {
    -                throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to reconnect host");
    -            }
    -        } catch (Exception ex) {
    -            s_logger.warn("Exception: ", ex);
    -            throw new ServerApiException(ApiErrorCode.RESOURCE_UNAVAILABLE_ERROR, ex.getMessage());
    +            Host result =_resourceService.reconnectHost(this);
    --- End diff --
    
    Great, thanks for the explanation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #837: CLOUDSTACK-8855 Improve Error Message for Host...

Posted by bvbharatk <gi...@git.apache.org>.

Github user bvbharatk commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/837#discussion_r105881440
  
    --- Diff: plugins/network-elements/netscaler/src/com/cloud/network/element/NetscalerElement.java ---
    @@ -512,7 +512,11 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
             });
             HostVO host = _hostDao.findById(lbDeviceVo.getHostId());
     
    -        _agentMgr.reconnect(host.getId());
    +        try {
    +            _agentMgr.reconnect(host.getId());
    +        } catch (Exception e ) {
    --- End diff --
    
    Using a more specific catch makes sense, I will take this up in a separate PR. This was supposed to be a small improvement to the existing code.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #837: CLOUDSTACK-8855 Improve Error Message for Host...

Posted by rafaelweingartner <gi...@git.apache.org>.

Github user rafaelweingartner commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/837#discussion_r105942978
  
    --- Diff: engine/components-api/src/com/cloud/agent/AgentManager.java ---
    @@ -141,7 +142,7 @@
     
         public void pullAgentOutMaintenance(long hostId);
     
    -    boolean reconnect(long hostId);
    +    void reconnect(long hostId) throws CloudRuntimeException, AgentUnavailableException;
    --- End diff --
    
    @bvbharatk I got your intentions here. However, don\u2019t you think we can work out the places where people are login runtime exceptions, instead of bubbling them up? I believe that instead of creating constraints in the code, we should spot when people are introducing these things, and educate them. For the code that is already doing the things you mentioned, well, we can work to fix them bit by bit. 
    
    It feels a little unusual (at least for me) to declare runtime exceptions. At the end of the day, you may make it more visible, but just because you declare a runtime exception, it does not mean people will be forced to catch it (they will not be forced to catch it).
    
    BTW: I never had a good expression to talk about the re-throw of exceptions. Thanks for the \u201cbubble exception up\u201d.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #837: CLOUDSTACK-8855 Improve Error Message for Host...

Posted by rafaelweingartner <gi...@git.apache.org>.

Github user rafaelweingartner commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/837#discussion_r106188366
  
    --- Diff: server/src/com/cloud/alert/AlertManagerImpl.java ---
    @@ -767,7 +767,9 @@ public void sendAlert(AlertType alertType, long dataCenterId, Long podId, Long c
                     // set up a new alert
                     AlertVO newAlert = new AlertVO();
                     newAlert.setType(alertType.getType());
    -                newAlert.setSubject(subject);
    +                //do not have a seperate column for content.
    +                //appending the message to the subject for now.
    +                newAlert.setSubject(subject+content);
    --- End diff --
    
    I agree with you that we should not try to save the world in a single day. However, comments in the code for me mean nothing (comments and documentation are two different things). ACS has a lot of comments saying to improve this or that, and they just stay there for days, months and years. So, adding a comment like that does not bring any value to the code. My point is that this specific case is not a complicated one; it is only a matter of adding a column to a table, then a new property in a POJO, and setting the correct value in the newly created property.
    
    The same way I understand that we should not try to save the planet at once, I also have the philosophy that we should not let to do tomorrow what can be done today.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #837: CLOUDSTACK-8855 Improve Error Message for Host...

Posted by sureshanaparti <gi...@git.apache.org>.

Github user sureshanaparti commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/837#discussion_r105888712
  
    --- Diff: engine/orchestration/src/com/cloud/agent/manager/AgentManagerImpl.java ---
    @@ -1049,7 +1044,13 @@ public boolean executeUserRequest(final long hostId, final Event event) throws A
                 }
                 return true;
             } else if (event == Event.ShutdownRequested) {
    -            return reconnect(hostId);
    +            //should throw a exception here as well.instead of eating this up.
    +           try {
    +               reconnect(hostId);
    +           } catch (CloudRuntimeException e) {
    --- End diff --
    
    @bvbharatk Thanks for the clarification.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #837: CLOUDSTACK-8855 Improve Error Message for Host...

Posted by bvbharatk <gi...@git.apache.org>.

Github user bvbharatk commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/837#discussion_r106089352
  
    --- Diff: server/src/com/cloud/alert/AlertManagerImpl.java ---
    @@ -767,7 +767,9 @@ public void sendAlert(AlertType alertType, long dataCenterId, Long podId, Long c
                     // set up a new alert
                     AlertVO newAlert = new AlertVO();
                     newAlert.setType(alertType.getType());
    -                newAlert.setSubject(subject);
    +                //do not have a seperate column for content.
    +                //appending the message to the subject for now.
    +                newAlert.setSubject(subject+content);
    --- End diff --
    
    @rafaelweingartner 
    I agree with you that it is a half measure, And so i commented "appending the message to the subject **for now**."  The idea was just to improve it little by little. We need not do every thing in this PR. We can work on it in a separate one.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #837: CLOUDSTACK-8855 Improve Error Message for Host...

Posted by sureshanaparti <gi...@git.apache.org>.

Github user sureshanaparti commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/837#discussion_r105722036
  
    --- Diff: engine/orchestration/src/com/cloud/agent/manager/AgentManagerImpl.java ---
    @@ -1049,7 +1044,13 @@ public boolean executeUserRequest(final long hostId, final Event event) throws A
                 }
                 return true;
             } else if (event == Event.ShutdownRequested) {
    -            return reconnect(hostId);
    +            //should throw a exception here as well.instead of eating this up.
    +           try {
    +               reconnect(hostId);
    +           } catch (CloudRuntimeException e) {
    --- End diff --
    
    @bvbharatk Is it possible to take the failure reason forward?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---