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

Remove unused classes and file #246

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Remove unused classes and file #246

merged 1 commit into from
Feb 21, 2024

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Feb 13, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Feb 13, 2024
@mosabua
Copy link
Member

mosabua commented Feb 13, 2024

Doesnt that remove actual functionality of using such a rule as in the example @willmostly @vishalya @Chaho12 ...

I think we probably want this kind of feature in the pluggable router modules you are working on @vishalya right?

@willmostly
Copy link
Contributor

Doesnt that remove actual functionality of using such a rule as in the example @willmostly @vishalya @Chaho12 ...

The test doesn't accurately reflect current functionality. In particular, the test adds this connection checker class to the request as an attribute, which makes it available for use in a rule. There are no implementations of RoutingGroupSelector.findRoutingGroup that do the same however.

HttpServletRequest mockRequest = mock(HttpServletRequest.class);
when(mockRequest.getAttribute("connectionChecker")).thenReturn(checker);

The proper way to add this would be as a fact to the rules engine instead of adding it to the request.

@vishalya I don't see this code in the old lyft gateway, but it was in the initial commit. Were you guys leveraging it at some point?

Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

This is directionally aligned with our initiative to make the routing interface more pluggable. This TCP Check functionality isn't used today, but if it is needed in the future a pluggable interface would allow it to easily be added back in.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Thanks @willmostly for confirming that it was needed / on our horizon but we can kill it off for now.

@ebyhr ebyhr merged commit e2aaa0e into trinodb:main Feb 21, 2024
2 checks passed
@ebyhr ebyhr deleted the ebi/remove-unused-class branch February 21, 2024 23:45
@github-actions github-actions bot added this to the 7 milestone Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants