[FLINK-10748] Decouple RestServerEndpoint from Dispatcher #7295
Conversation
This commits decouples the RestServerEndpoint from the Dispatcher by converting the RedirectHandler into LeaderRetrievalHandler which only retrieves the gateway of the current leader instead of redirecting the client. This is possible since the serving RestServerEndpoint no longer needs to be colocated with the Dispatcher and JobMaster after all RPC requests are serializable (e.g. no direct access on the ExecutionGraph is needed). With this change every RestServerEndpoint will automatically proxy to the current leader without needing to redirect the requesting client.
| public static HttpResponse getRedirectResponse(String redirectAddress, String path) { | ||
| return getRedirectResponse(redirectAddress, path, HttpResponseStatus.TEMPORARY_REDIRECT); | ||
| } | ||
|
|
||
| public static HttpResponse getRedirectResponse(String redirectAddress, String path, HttpResponseStatus code) { |
There was a problem hiding this comment.
I supposed the remaining ones are used for the SSL redirection?
| try { | ||
| respondAsLeader(channelHandlerContext, routedRequest, gateway); | ||
| } catch (Exception e) { | ||
| logger.error("Error while responding as leader.", e); |
There was a problem hiding this comment.
this isn't quite correct is it.
There was a problem hiding this comment.
Do you suggest to log a different error message? This is the same message as it was before. We could change it to Error occurred while processing http request.
There was a problem hiding this comment.
I'd prefer that. It's a bit....well we just aren't really the leader (in a way this concept no longer exists on the REST handler level), so it seems a bid misleading.
There was a problem hiding this comment.
Alright, will change it.
| Assert.assertEquals(expectedRedirection, response.getLocation()); | ||
|
|
||
| // 4. with local REST endpoint | ||
| // 2. with leader |
There was a problem hiding this comment.
help me out here, why is the leader now suddenly available?
There was a problem hiding this comment.
with the removal of the redirection logic, there are only two cases:
- Leader could not be retrieved yet
- Leader has been retrieved
These two cases are tested.
There was a problem hiding this comment.
oh i get that, but this test only executes
httpClient.sendGetRequest(restPath, FutureUtils.toFiniteDuration(timeout));
HttpTestClient.SimpleHttpResponse response = httpClient.getNextResponse(FutureUtils.toFiniteDuration(timeout));
twice, with each getting a different response. What causes the response to be different? I don't see any modifications being made to the state of the rest server / futures between the 2 requests. This isn't just a timing thing, is it?
There was a problem hiding this comment.
No, it is the setup of the test with the gatewayRetriever mock in line 73. First we return an empty Optional and then an Optional.of(gateway).
There was a problem hiding this comment.
I actually removed the mocking completely. This should make things a bit clearer.
|
Thanks for the review @zentol. Merging this PR. |
This commits decouples the RestServerEndpoint from the Dispatcher by converting the RedirectHandler into LeaderRetrievalHandler which only retrieves the gateway of the current leader instead of redirecting the client. This is possible since the serving RestServerEndpoint no longer needs to be colocated with the Dispatcher and JobMaster after all RPC requests are serializable (e.g. no direct access on the ExecutionGraph is needed). With this change every RestServerEndpoint will automatically proxy to the current leader without needing to redirect the requesting client.
What is the purpose of the change
This commits decouples the RestServerEndpoint from the Dispatcher by converting the
RedirectHandler into LeaderRetrievalHandler which only retrieves the gateway of the
current leader instead of redirecting the client. This is possible since the serving
RestServerEndpoint no longer needs to be colocated with the Dispatcher and JobMaster
after all RPC requests are serializable (e.g. no direct access on the ExecutionGraph
is needed).
With this change every RestServerEndpoint will automatically proxy to the current leader
without needing to redirect the requesting client.
Brief change log
RedirectHandlerRedirectHandlerintoLeaderRetrievalHandlerrestAddressFuturefrom all REST handlers except for theJarListHandlerVerifying this change
RedirectHandlerTestwhich is now calledLeaderRetrievalHandlerTestDoes this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation