[v2] node/relay bug fixes #908
Conversation
ian-shim
force-pushed
the
v2-node-relay-fixes
branch
from
e60aa2a to
1fe0be4
Compare
| initOnce *sync.Map | ||
| // conns maps relay key to the gRPC connection: `map[corev2.RelayKey]*grpc.ClientConn` | ||
| conns sync.Map | ||
| logger logging.Logger | ||
|
|
||
| // grpcClients maps relay key to the gRPC client: `map[corev2.RelayKey]relaygrpc.RelayClient` | ||
| grpcClients sync.Map |
There was a problem hiding this comment.
All maps need to be thread safe
| @@ -119,8 +127,7 @@ func (m *metadataProvider) GetMetadataForBlobs(keys []v2.BlobKey) (metadataMap, | |||
| }() | |||
| } | |||
|
|
|||
| mMap := make(metadataMap) | |||
| for len(mMap) < len(keys) { | |||
There was a problem hiding this comment.
This hangs if there are duplicates in keys
| @@ -119,8 +127,7 @@ func (m *metadataProvider) GetMetadataForBlobs(keys []v2.BlobKey) (metadataMap, | |||
| }() | |||
| } | |||
|
|
|||
| mMap := make(metadataMap) | |||
| for len(mMap) < len(keys) { | |||
node/grpc/server_v2_test.go
Outdated
| config.EnableV2 = false | ||
| c := newTestComponents(t, config) | ||
| _, err := c.server.StoreChunks(context.Background(), &pbv2.StoreChunksRequest{}) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
These block of code may be benefited from a small helper function (parse error and assert the error status), as they repeat many times in this file
| logger logging.Logger | ||
|
|
||
| // grpcClients maps relay key to the gRPC client: `map[corev2.RelayKey]relaygrpc.RelayClient` | ||
| grpcClients sync.Map | ||
| } | ||
|
|
||
| var _ RelayClient = (*relayClient)(nil) | ||
|
|
||
| // NewRelayClient creates a new RelayClient that connects to the relays specified in the config. | ||
| // It keeps a connection to each relay and reuses it for subsequent requests, and the connection is lazily instantiated. |
There was a problem hiding this comment.
Why lazy init? It's not that expensive to create a connection once per Node creation
There was a problem hiding this comment.
Just preventing a case that creates a large number of connections. This client is not necessarily only used by the node as it's meant to be used as a library
node/grpc/server_test.go
Outdated
| @@ -83,6 +83,7 @@ func makeConfig(t *testing.T) *node.Config { | |||
| DbPath: t.TempDir(), | |||
| ID: opID, | |||
| NumBatchValidators: runtime.GOMAXPROCS(0), | |||
| EnableV2: true, | |||
There was a problem hiding this comment.
This removes the test coverage for the existing v1, we need to keep testing it
There was a problem hiding this comment.
Sounds good. Defaulted it to false and overwrote it to true in server_v2_test
ian-shim
force-pushed
the
v2-node-relay-fixes
branch
from
1fe0be4 to
2647dec
Compare
ian-shim
force-pushed
the
v2-node-relay-fixes
branch
from
2647dec to
1958f6d
Compare
ian-shim
force-pushed
the
v2-node-relay-fixes
branch
from
1958f6d to
00c7c50
Compare
Why are these changes needed?
A number of bug fixes in relay and v2 node.
Also, it adds a feature flag for enabling v2 features on the node.
Checks