refactoring in port availability check function, added one more function get common formatted error which can be printed on console #79
Conversation
…ion get common formatted error which can be printed on console
plenum/common/util.py
Outdated
| "got error: {}".format(str(e)) | ||
| logging.warning(erroMsg) | ||
| raise e | ||
|
|
There was a problem hiding this comment.
This does not separate concerns. checkPortAvailable should not be concerned with logging at all. Raise an exception and let application fault barriers handle logging.
|
See comment in commit. Can't accept this code as is. |
|
Were we working from a broken tests? If not, I would like to see a test with this check in proving this behavior. |
|
+1 to @devin-fisher comment. We need a failing test that is resolved with this change. Also, it doesn't appear any changes have happened since my comments yesterday. |
…ied checkPortAvailable to throw approprite exceptions
… into agent-startup-refactoring
… console, refactored checkPortAvailable function as well
plenum/common/constants.py
Outdated
| @@ -0,0 +1 @@ | |||
| SOCKET_BIND_ERROR_ALREADY_IN_USE = 98 | |||
There was a problem hiding this comment.
Lets separate constants by their purpose, at least the ones which indicate error codes, so lets have a module called error_codes with such constants, later we can have error messages with a map with keys as above constants
|
|
||
|
|
||
| class PortNotAvailable(Exception): | ||
| def __init__(self, port): |
There was a problem hiding this comment.
port should be part of exception args
plenum/common/util.py
Outdated
| @@ -599,3 +603,9 @@ def check_endpoint_valid(endpoint, required: bool=True): | |||
| raise InvalidEndpointIpAddress(endpoint) from exc | |||
| if not (port.isdigit() and int(port) in range(1, 65536)): | |||
There was a problem hiding this comment.
Can you make a method called valid port which covers these cases, this method can be useful at other places too.
|
|
||
| def getFormattedErrorMsg(msg): | ||
| msgHalfLength = int(len(msg) / 2) | ||
| errorLine = "-" * msgHalfLength + "ERROR" + "-" * msgHalfLength |
There was a problem hiding this comment.
Why half length, whats wrong with a fixed number of characters or some percentage of screen width. It would look odd when you print 2 error messages one below the other where one of them is significantly different in size
plenum/test/helper.py
Outdated
| @@ -534,15 +534,20 @@ def stopNodes(nodes: List[TestNode], looper=None, ensurePortsFreedUp=True): | |||
| assert looper, 'Need a looper to make sure ports are freed up' | |||
| for node in nodes: | |||
| node.stop() | |||
|
|
|||
| ports = [[n.nodestack.ha[1], n.clientstack.ha[1]] for n in nodes] | |||
There was a problem hiding this comment.
Why do this computation before you know that "port freeness" needs to be ensured? And why burden waitUntillPortIsAvailable with the concern that it has to flatten the list of ports, lets keep it simple and do the flattening here
ghost
unassigned 
Add new method getNodesServices (hyperledger#506)
No description provided.