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

    https://github.com/apache/cloudstack/pull/238#discussion_r31114895
  
    --- Diff: 
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConsoleProxyLoadCommandWrapper.java
 ---
    @@ -49,6 +49,11 @@ public Answer executeProxyLoadScan(final Command cmd, 
final long proxyVmId, fina
                 final URL url = new URL(sb.toString());
                 final URLConnection conn = url.openConnection();
     
    +            // setting TIMEOUTs to avoid possible waiting until death 
situations
    --- End diff --
    
    Hi @rsafonseca 
    
    Will you remove the changes from you PR and I add to mine, or should we 
wait for your stuff to get merged?
    
    A couple of question concerning this change:
    
    1. When you run the Unit Tests having this timeout, how do they perform?
    2. Before your changes get through, would you mind replacing the magic 
number by a static final variable with a clear description of the timeout? It 
could be added to the LibvirtUtilitiesHelper.
    
    If the tests get somehow slower due to this, we can also add an accessor 
method to the timeout value. By doing so we can mock the call and when running 
the tests we can intercept it and return 0, instead of 5000.
    
    Cheers,
    Wilder


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to