Fix/#415 dynamic port binding #417
Conversation
…am from classpath or default
…fix/#415-dynamic-port-binding fix dynamic port binding
Codecov Report
@@ Coverage Diff @@
## develop #417 +/- ##
=============================================
- Coverage 49.47% 49.42% -0.06%
Complexity 1649 1649
=============================================
Files 240 240
Lines 10432 10443 +11
Branches 1498 1501 +3
=============================================
Hits 5161 5161
- Misses 4654 4665 +11
Partials 617 617
Continue to review full report at Codecov.
|
|
@jiachen1120 I think minPort should be ok to equal maxPort, just when it's strictly greater should there be an error |
|
@NicholasAzar I also think it should be ok to equal in theory, but the previous performance is that the range of the port is: minPort <= port < maxPort. If let minPort equal to maxPort, no error message appear and no port bound successfully. So in order to make the previous performance unchanged I make such kind of change. @stevehu Or, maybe should we change the port range to |
|
@jiachen1120 @NicholasAzar We are discuss the edge case now. In most of the cases, users won't use dynamic port if the minPort and maxPort is equal. They would assign the port directly without dynamic port enabled. That being said, we cannot predict user behaviors. In the case that minPort is equal to maxPort, we should try to bind to it and quit with error if that particular port is not availabe. If that means we have to break the previous behavior, then let's break it as I don't think anybody is depending on the behavior. I think we should change the port range to minPort <= port <= maxPort as @jiachen1120 suggested. |
* - overload getConfigStream method in config module to get config stream from classpath or default * revert * Service terminate with error message when minPort larger than or equal to maxPort * Changed dynamic port interval
@NicholasAzar @stevehu Hey! I am sorry that I have not fulfilled my duties to complete the task you handed in. I just did a test and found that there may still be some areas to improve on this issue.
I just did a manual test of develop branch and found the following problems:
If the user is using a static port and the port happens to be occupied, the user will get the following information:
Fail to bind to port 8453, trying 8454
And at this point the server does not stop, perhaps this will make the user mistakenly think that the server is always trying to bind the port. I think we should let the exception throw at the moment.
Then, I test @NicholasAzar changes. due to the whole bind() is put in to try catch block, the registration exception would be caught by outer try-catch and the user would get the port bind fail message when the registration fails.
In summary, I made some changes to make sure the following performance:
Related topics:
#416
#415