CLOUDSTACK-8867: Added retry logic to reconnect to host on connection termination to console #1269
Conversation
|
@anshul1886 could you please create a method (with documentation) for those duplicate blocks in ConsoleProxy ( "if(viewer != null && !(viewer.getClientHostPort() == param.getClientHostPort())) {...}", lines 414-419 and 454-458 )? |
|
@GabrielBrascher That block of code is in synchronised block. |
|
@anshul1886 I don't think extracting that piece of code into another method would affect the lock on the coneectionMap variable. A method invocation inside a syncrhonized block acts just like regular code, but extracting it into a new method would reduce the code length and enhance reusabilty. According to Javadocs https://docs.oracle.com/javase/tutorial/essential/concurrency/syncmeth.html the lock is held until the end of the synchronized block. This question on stackoverflow may be old but illustrates this same situation: http://stackoverflow.com/questions/6961848/calling-a-method-from-within-a-synchronized-method |
|
@anshul1886 please rebase against latest master |
|
I am missing code reviews on this one... |
|
Can I get some code review on this? Thanks... |
|
tag:needsreview |
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests: Skipped tests: Passed test suits: |
… termination to console. It also improves handling of sessions to hypervisor in console proxy
anshul1886
force-pushed
the
CLOUDSTACK-8867-2
branch
from
0f724d5
to
79e6d9f
Compare
|
@rhtyd , Rebased against latest master |
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests:
Skipped tests: Passed test suits: |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-547 |
|
@GabrielBrascher @alexandrelimassantana I kept it that way for readability as console proxy code is hard to debug. |
|
Thanks for the response @anshul1886. I am sorry, but I disagree with you. I think that it gets more stable and readable with less duplicated code. In my humble opinion, it is easier to document, test, read and debug when we have lots of small methods than few big methods. |
|
@GabrielBrascher The other method is there for legacy purposes and is no longer get used in Cloudstack. This was getting used in 2.x versions for UI. Some third party tool might be using it so kept it for that purpose. Mixing things will make it more complicated. |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-845 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1243)
|
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1402 |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1407 |
|
Is it reasonable to block a merge on this because of a coding style disagreement? Also I agree with @anshul1886 that it seems to make the code harder to understand if one would create a helper function because of five line of duplicated code. Especially because there is no obvious way to name the new function so it would make sense semantically. At least from my point of understanding. Maybe you have some suggestion on how to name the function? @GabrielBrascher |
|
@DennisKonrad the PR is not blocked because of coding style. The author simply did not provide a technical reason to leave it as is. The problem here is also not about function naming. The issue here goes beyond that, and it has to do with code quality. ACS project has been suffering for years because of duplicated and convoluted code (bad practices that were implemented along the years). I only started a discussion regarding technical problems I found in this PR. Although I disagree with his approach, I did not block this PR (I do not even have powers to do that). The problem here is that the author did not care enough to provide a reasonable answer to maintain the duplicated code, or to reduce the code duplication, document it and create unit-tests for the newly added block of code. |
|
@anshul1886 please rebase and re-open if still relevant |
It also improves handling of sessions to hypervisor in console proxy
https://issues.apache.org/jira/browse/CLOUDSTACK-8867
To test:
Try on XenServer setup as frequent disconnections are more common for XenServer.
Try random key combinations on VM console and see after fix console access denied errors are less frequent.