Conversation
absurdfarce
left a comment
absurdfarce
left a comment
There was a problem hiding this comment.
Thanks for the PR @wuzhongdehua!
The main things for me on an initial pass would be a preference for namedtuples instead of conventional tuples and the comments around the API changes.
| if type(translated_address) is tuple and len(translated_address)>1: | ||
| ip = translated_address[0] | ||
| port = translated_address[1] | ||
| log.debug( "PANDU: CREATING Default endpoint {} {}".format(ip, port)) |
There was a problem hiding this comment.
This debug line should also be removed
| # TODO next major, create a TranslatedEndPoint type | ||
| ## Overriden by Pandu. using ip + port translation | ||
| translated_address = self.cluster.address_translator.translate(addr) | ||
| log.debug("PANDU: translated address {}".format(translated_address)) |
There was a problem hiding this comment.
This debug line should be removed
| ## Overriden by Pandu. using ip + port translation | ||
| translated_address = self.cluster.address_translator.translate(addr) | ||
| log.debug("PANDU: translated address {}".format(translated_address)) | ||
| if type(translated_address) is tuple and len(translated_address)>1: |
There was a problem hiding this comment.
I think I'd prefer a namedtuple here rather than a straight tuple... just to avoid any possible confusion with positions in a base tuple.
| # create the endpoint with the translated address | ||
| # TODO next major, create a TranslatedEndPoint type | ||
| ## Overriden by Pandu. using ip + port translation | ||
| translated_address = self.cluster.address_translator.translate(addr) |
There was a problem hiding this comment.
It seems really odd to me that this changes the API from address -> translated address to address -> (translated address, translated port). It's not immediately obvious to me how many use cases would be able to compute a translation for port numbers based only on an input address. Seems like the more complete option would be to provide both address and port if we're going to return a combination of translated address and port.
This also probably helps out with the API. We have something like this right now:
def translate(self, addr) -> str:
The proposal here is to move to something like this:
def translate(self, addr) -> (str | [tuple[str,int])
This is an API change but it's an addition rather than a change of existing functionality; we won't break any address translators out there. But if we add a new method which clearly returns a host/string tuple (of some form) then we can clearly distinguish between these cases:
def translate(self, addr) -> str:
...
def translate_all(self, addr, port) -> tuple[str,int]
We'd still have the question of how we want DefaultEndPointFactory to use these methods... but it might be clearer if we did so?
|
@bschoening I'm curious what you think about this one. |
|
@wuzhongdehua can you add a description to this PR? I don't understand why a tuple format is preferable and turns one line into ten. |
4 participants
No description provided.