Remote shard mapping over TCP #3376
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
| @@ -26,13 +26,13 @@ func (a mapperValues) Len() int { return len(a) } | |||
| func (a mapperValues) Less(i, j int) bool { return a[i].Time < a[j].Time } | |||
| func (a mapperValues) Swap(i, j int) { a[i], a[j] = a[j], a[i] } | |||
|
|
|||
| type mapperOutput struct { | |||
| type MapperOutput struct { | |||
There was a problem hiding this comment.
Simply exporting this type, no other changes.
otoolep
force-pushed
the
mapping_over_tcp
branch
2 times, most recently
from
813ad49 to
ab53bdc
Compare
| err := s.processMapShardRequest(conn, buf) | ||
| if err != nil { | ||
| s.Logger.Printf("process map shard error: %s", err) | ||
| _ = writeMapShardResponseMessage(conn, NewMapShardResponse(1, err.Error())) |
There was a problem hiding this comment.
Error ignored. Should at least log it.
|
A few small things but 👍 after they're fixed. |
|
Thanks for the feedback @jwilder |
With this change remote mapping no longer uses HTTP, as the HTTP ports exposed by nodes on the cluster are not known cluster wide. The TCP ports exposed by the cluster service are, so this change uses that functionality. Each RemoteMapper has its own dedicated connection pool for each node, and remote mapping TCP connections are in no way coupled with query TCP connections.
This is useful primarily for testing.
otoolep
force-pushed
the
mapping_over_tcp
branch
from
b28f268 to
10eecb4
Compare
|
OK, I believe I addressed all comments, merging now |
|
@jwilder - I just realised I merged the protobuf changes that you pointed out. I did not mean to do that. I meant to handle that in a separate PR, as you requested. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this change remote mapping no longer uses HTTP, as the HTTP ports exposed by nodes on the cluster are not known cluster wide. The TCP ports exposed by the cluster service are, so this change uses that functionality. Each RemoteMapper has its own dedicated connection pool for each node, and remote mapping TCP connections are in no way coupled with query TCP connections.
It is not clear to me if this will be part of 0.9.2, so I have left the CHANGELOG untouched for now.
I tested this code by forcing all mapping to take place over the network, spun up a single node, and performed some queries. This meant shard mapping actually took place over localhost. The returned data looked good.
The structure is as follows.
ShardMapperfor 1 or more shards. This takes place on the Co-ordinator node.ShardMapperreturns an object implementing theMapperinterface.RemoteMapper.RemoteMapperconnects to the target node, sending the shard ID and query to the remote node.RemoteMapperdecode results and presents them to the query engine.