feat(scattergather): Initial scatter/gather library #3173
Conversation
robzienert
force-pushed
the
scattergather
branch
from
3efaf54
to
600981c
Compare
|
Looks like I broke all the things when I split this out from the original codebase. 🤦♂️ |
robzienert
force-pushed
the
scattergather
branch
2 times, most recently
from
21cd404
to
7867387
Compare
robzienert
force-pushed
the
scattergather
branch
3 times, most recently
from
d6b60c2
to
9fc70db
Compare
There was a problem hiding this comment.
looks pretty straightforward, I like it
the strategy is basically we take a request and pass it everywhere, and the destinations have to be smart enough to noop it if its not applicable to them (e.g. shard on region, we are going to send a get request everywhere and only the shards that handle that region are going to return results, and then the deep merge just merges a bunch of empty results with the actual results?)
| } else if (valueToBeUpdated != null && valueToBeUpdated.isObject) { | ||
| mergeNodes(valueToBeUpdated, updatedValue) | ||
| } else { | ||
| // TODO(rz): Replace is not correct behavior |
There was a problem hiding this comment.
what would you expect the correct behaviour to be here? (replace seems legit to me so I am not entirely sure what I am missing)
There was a problem hiding this comment.
I forgot to remove that comment, it was behaving differently before.
| distinctCodes.size == 1 -> distinctCodes[0] | ||
| distinctCodes.any { it == 404 } -> HttpStatus.NOT_FOUND.value() | ||
| distinctCodes.any { it == 429 } -> HttpStatus.TOO_MANY_REQUESTS.value() | ||
| else -> distinctCodes.sortedDescending().first() |
There was a problem hiding this comment.
not sure this is the right default case here - thinking that one node returns a 400 for some reason and the others return 200, probably needs to make sure the JSON error response doesn't get merged with the 200 success responses
There was a problem hiding this comment.
I'm going to change the logic so that the reducer will extract the response from the highest error code if it's not 2xx.
| get { characterEncoding }.isEqualTo("UTF-8") | ||
| get { body }.isEqualTo(expectedBody) | ||
| get { isError }.isEqualTo(false) | ||
| } |
|
@cfieber The smarts of what shards do what is really not defined by this library. In the federation implementation, the federator actually has the smarts to determine which shards need to handle a request. That said, yes, all downstream servers will need to at least know about the scatter/gather header if they're also running the federation code. |
robzienert
force-pushed
the
scattergather
branch
from
9fc70db
to
488f5c6
Compare
Development implementation of a scatter/gather library, for future use in clouddriver federation, allowing operators to shard write & read traffic without requiring development work on other services. It's possible this behavior could be lifted to its own federation service in the future if other services need the same behavior. This API is currently coupled to `HttpServletRequest`, which it will then fanout into a collection of OkHttp `Call` objects that will be executed according to the provided `ScatterGather` implementation. Currently only a `NaiveScatterGather` is available, which performs the calls sequentially and without any error handling: This is for development purposes only. Followup PRs will be made to add a `ScatterGather` implementation that executes requests in parallel, performs error handling & retries as well as cancelations in the event of timeouts. Responses from shards are reduced into a single `ReducedResponse` by a pluggable `ResponseReducer`. The existing implementation is a simple deep merge reducer. Better to place this in kork? There's nothing specific to clouddriver in this library, nor do I expect that there ever will be.
robzienert
force-pushed
the
scattergather
branch
from
488f5c6
to
007d2b6
Compare
Development implementation of a scatter/gather library, for future use in clouddriver federation, allowing operators to shard write & read traffic without requiring development work on other services. It's possible this behavior could be lifted to its own federation service in the future if other services need the same behavior. This API is currently coupled to `HttpServletRequest`, which it will then fanout into a collection of OkHttp `Call` objects that will be executed according to the provided `ScatterGather` implementation. Currently only a `NaiveScatterGather` is available, which performs the calls sequentially and without any error handling: This is for development purposes only. Followup PRs will be made to add a `ScatterGather` implementation that executes requests in parallel, performs error handling & retries as well as cancelations in the event of timeouts. Responses from shards are reduced into a single `ReducedResponse` by a pluggable `ResponseReducer`. The existing implementation is a simple deep merge reducer. Better to place this in kork? There's nothing specific to clouddriver in this library, nor do I expect that there ever will be.
Fix for inbound/outbound rules only detected in apiVersion=network.k8s.io/v1
Development implementation of a scatter/gather library, for future use in
clouddriver federation, allowing operators to shard write & read traffic
without requiring development work on other services. It's possible this
behavior could be lifted to its own federation service in the future if
other services need the same behavior.
This API is currently coupled to
HttpServletRequest, which it will thenfanout into a collection of OkHttp
Callobjects that will be executedaccording to the provided
ScatterGatherimplementation. Currently only aNaiveScatterGatheris available, which performs the calls sequentiallyand without any error handling: This is for development purposes only.
Followup PRs will be made to add a
ScatterGatherimplementation thatexecutes requests in parallel, performs error handling & retries as well
as cancelations in the event of timeouts.
Responses from shards are reduced into a single
ReducedResponseby apluggable
ResponseReducer. The existing implementation is a simpledeep merge reducer.
Better to place this in kork? There's nothing specific to clouddriver in
this library, nor do I expect that there ever will be.