Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix query ui history redirect #193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

regadas
Copy link

@regadas regadas commented Feb 5, 2023

Hello!

This addresses the issue in #162 here /ui/query?<QUERYID> is redirected by default to adhoc

@regadas regadas force-pushed the fix_162 branch 3 times, most recently from ae80dfe to 49dced5 Compare February 5, 2023 15:36
String routingGroup = routingGroupSelector.findRoutingGroup(request);
String user = Optional.ofNullable(request.getHeader(USER_HEADER))
.orElse(request.getHeader(ALTERNATE_USER_HEADER));
if (!Strings.isNullOrEmpty(routingGroup)) {
// This falls back on adhoc backend if there are no cluster found for the routing group.
backendAddress = routingManager.provideBackendForRoutingGroup(routingGroup, user);
return routingManager.provideBackendForRoutingGroup(routingGroup, user);
Copy link
Contributor

@puneetjaiswal puneetjaiswal Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please dont return from here. Below lines are skipped. without this, subsequent calls would require a broadcast to all backends for query id lookup.

       // set target backend so that we could save queryId to backend mapping later.
      ((MultiReadHttpServletRequest) request).addHeader(PROXY_TARGET_HEADER, backendAddress);

would be nice if you could add a test case validating the presence of this header in the response.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @puneetjaiswal thanks for taking a look. The above return is within the orElseGet lambda for the backendAddress assignment. Effectively, L137 doesn't get skipped. I'll add a test.

@Pluies
Copy link
Contributor

Pluies commented Feb 16, 2023

@regadas heads up, this fails mvn test with:

[ERROR] Failures:
[ERROR]   TestQueryIdCachingProxyHandler.testExtractQueryIdFromUrl:28 expected [Optional[20200416_160256_03078_6b4yt]] but found [Optional.empty]

@regadas
Copy link
Author

regadas commented Feb 17, 2023

@Pluies yup indeed it was faulty, somehow I missed that. Thanks for pointing that out!

@Chaho12
Copy link

Chaho12 commented Jun 16, 2023

Hi, it would be great if this PR gets merged !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants