Structify Opeartor Socket #1326
Conversation
supriya-premkumar
force-pushed
the
supriya-egda-834-refactor-structify-operatorsocket
branch
2 times, most recently
from
4d165f9 to
c5e5ec1
Compare
| v1RetrievalPort string | ||
| v2DispersalPort string | ||
| v2RetrievalPort string | ||
| } | ||
|
|
||
| func (s OperatorSocket) String() string { |
There was a problem hiding this comment.
Marshal/Unmarshal or Serialize/Deserialize? String() feels like producing something for logging, debugging or display.
| //TODO: Add config checks for invalid v1/v2 configs -- for v1, both v2 ports must be empty and for v2, both ports must be valid, reject any other combinations. | ||
| if v2DispersalPort == "" && v2RetrievalPort == "" { | ||
| return OperatorSocket(fmt.Sprintf("%s:%s;%s", nodeIP, dispersalPort, retrievalPort)) | ||
| func NewOperatorSocket(nodeIP, dispersalPort, retrievalPort, v2DispersalPort, v2RetrievalPort string) OperatorSocket { |
There was a problem hiding this comment.
nit: move factory method right above (before String())
| if err != nil { | ||
| logger.Warn("Failed to parse operator socket", "err", err) | ||
| } | ||
| socket = operatorSocket.GetV1RetrievalSocket() |
There was a problem hiding this comment.
If err != nil, how can this still get v1 retrieval socket?
| operatorSocket := core.OperatorSocket(operatorInfo.Socket) | ||
| operatorSocket, err := core.ParseOperatorSocket(operatorInfo.Socket) | ||
| if err != nil { | ||
| oh.logger.Warn("failed to parse operator socket", "operatorId", operatorId, "error", err) |
There was a problem hiding this comment.
In what cases it'll fail to parse? Is that just a warning issue?
Changes: Defines a concrete type OperatorSocket Internal state is unexported to prevent unintented changes from the importing package. Defines Get functions to access methods
supriya-premkumar
force-pushed
the
supriya-egda-834-refactor-structify-operatorsocket
branch
from
df5a7cc to
d431b1e
Compare
| @@ -40,8 +40,12 @@ func (c client) GetBlobHeader( | |||
| batchHeaderHash [32]byte, | |||
There was a problem hiding this comment.
instead of taking socket parameter as string, can we take it as OperatorSocket?
|
|
||
| func (s OperatorSocket) String() string { | ||
| return string(s) | ||
| if s.v2DispersalPort == "" && s.v2RetrievalPort == "" { | ||
| return fmt.Sprintf("%s:%s;%s", s.host, s.v1DispersalPort, s.v2RetrievalPort) |
There was a problem hiding this comment.
s/s.v2RetrievalPort/s.v1RetrievalPort
should it also check if v1 ports are there?
| if v2DispersalPort == "" && v2RetrievalPort == "" { | ||
| return OperatorSocket(fmt.Sprintf("%s:%s;%s", nodeIP, dispersalPort, retrievalPort)) | ||
| func NewOperatorSocket(nodeIP, dispersalPort, retrievalPort, v2DispersalPort, v2RetrievalPort string) OperatorSocket { | ||
| return OperatorSocket{ |
There was a problem hiding this comment.
should we add a validation here? (i.e. host & v1 ports cannot be empty, if v2DispersalPort exists, v2RetrievalPort must exist)
Changes:
Defines a concrete type OperatorSocket
Internal state is unexported to prevent unintented changes from the importing package.
Defines Get functions to access methods
Why are these changes needed?
Checks