Extracts the transformation logic on Future instances into a dedicated class #688

yaelselig30 · 2025-05-22T10:06:06Z

Description

This PR extracts the transformation logic on Future instances into a dedicated class to improve readability, enable easier extension, and make the logic more testable.

Additional context and related issues

Changes:

Introduced a new class, responsible for applying these transformations.
Moved transformation logic out of the handler to this class.

Benefits:

Easier to test transformation logic independently.
Better separation of concerns.
More maintainable structure for adding future behavior.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

cla-bot · 2025-05-22T10:06:08Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

ebyhr · 2025-05-22T22:49:20Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+    public ProxyResponseTransformer(QueryHistoryManager queryHistoryManager,
+                                    RoutingManager routingManager)


The format is wrong. It should be:

public ProxyResponseTransformer( QueryHistoryManager queryHistoryManager, RoutingManager routingManager)

ebyhr · 2025-05-22T22:49:41Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+        this.routingManager = requireNonNull(routingManager, "routingManager is null");
+        this.queryHistoryManager = requireNonNull(queryHistoryManager, "queryHistoryManager is null");


Replace the order of these lines for consistency with constructor parameter and fields.

ebyhr · 2025-05-22T22:54:33Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+    private static final Logger log = Logger.get(ProxyRequestHandler.class);
+    private final QueryHistoryManager queryHistoryManager;


Add an empty line to separate static final and final.

ebyhr · 2025-05-22T22:54:37Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+    public ProxyResponse transform(Request request, ProxyResponse response, @Nullable String username, RoutingDestination routingDestination)
+    {
+        return recordBackendForQueryId(request, response, username, routingDestination);
+    }
+
+    private ProxyResponse recordBackendForQueryId(Request request, ProxyResponse response,


Merge these methods into one.

The main reason for introducing ProxyResponseTransformer is to make it easier to extend the functionality of the FluentFuture.
With that in mind, I believe it's better to separate the transform method from the internal helper methods it relies on.
@ebyhr What do you think?

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

ebyhr · 2025-05-22T22:54:41Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+
+        if (response.statusCode() == OK.getStatusCode()) {
+            try {
+                HashMap<String, String> results = OBJECT_MAPPER.readValue(response.body(), HashMap.class);


Fix warning or add //noinspection unchecked.

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

ebyhr · 2025-05-22T22:56:45Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+            catch (IOException e) {
+                log.error("Failed to get QueryId from response [%s] , Status code [%s]", response.body(), response.statusCode());


Please include e in the logger message.

log.error(e, "Failed to get QueryId from response [%s] , Status code [%s]", response.body(), response.statusCode());

ebyhr · 2025-05-22T22:59:40Z

extract future of handle response to a new class

The commit title should start with an uppercase character: https://trino.io/development/process#pull-request-and-commit-guidelines-

cla-bot · 2025-05-25T08:18:52Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-05-25T08:35:00Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

yaelselig30 · 2025-05-27T11:18:57Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+
+public class ProxyResponseTransformer
+{
+    private static final Logger log = Logger.get(ProxyRequestHandler.class);


change to ProxyResponseTransformer
private static final Logger log = Logger.get(ProxyResponseTransformer.class);

andythsu · 2025-05-27T22:41:13Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyRequestHandler.java


    @Inject
    public ProxyRequestHandler(
            @ForProxy HttpClient httpClient,
-            RoutingManager routingManager,
-            QueryHistoryManager queryHistoryManager,
+            ProxyResponseTransformer proxyResponseTransformer,


I don't see how is ProxyResponseTransformer being provided

Copilot

Pull Request Overview

This PR extracts the transformation logic on Future instances into the new ProxyResponseTransformer class to improve readability and testability. Key changes include the extraction of query transformation logic from ProxyRequestHandler, removal of the inner ProxyResponse record in favor of a dedicated file, and overall cleanup of redundant logic.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java	New class for handling transformation and query detail recording
gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseHandler.java	Removed inner ProxyResponse record and tidied up unused imports
gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponse.java	New record to represent proxy responses
gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyRequestHandler.java	Refactored to utilize the new ProxyResponseTransformer and removed redundant logic

Copilot · 2025-06-02T07:45:23Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+
+public class ProxyResponseTransformer
+{
+    private static final Logger log = Logger.get(ProxyRequestHandler.class);


Logger in ProxyResponseTransformer should reference ProxyResponseTransformer.class instead of ProxyRequestHandler.class to ensure accurate logging context.

Suggested change

private static final Logger log = Logger.get(ProxyRequestHandler.class);

private static final Logger log = Logger.get(ProxyResponseTransformer.class);

Copilot · 2025-06-02T07:45:23Z

gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyResponseTransformer.java

+        QueryHistoryManager.QueryDetail queryDetail = new QueryHistoryManager.QueryDetail();
+        queryDetail.setBackendUrl(getRemoteTarget(request.getUri()));
+        queryDetail.setCaptureTime(System.currentTimeMillis());
+        if (Strings.isNullOrEmpty(username)) {


The condition appears inverted; username should be set only if it is not null or empty (i.e., use if (!Strings.isNullOrEmpty(username))).

Suggested change

if (Strings.isNullOrEmpty(username)) {

if (!Strings.isNullOrEmpty(username)) {

This change moves the transformation logic applied to Future instances into a separate class, making it easier to extend and maintain. It also helps clarify the responsibilities of each component involved.

cla-bot · 2025-06-03T14:18:37Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

andythsu · 2025-06-05T16:37:10Z

gateway-ha/src/main/java/io/trino/gateway/ha/module/HaGatewayProviderModule.java

@@ -83,6 +85,7 @@ public class HaGatewayProviderModule
    protected void configure()
    {
        jaxrsBinder(binder()).bindInstance(resourceSecurityDynamicFeature);
+        bind(ProxyResponseTransformer.class).in(Scopes.SINGLETON);


can we use optionalBinder() so that others can override it?

Suggested change

bind(ProxyResponseTransformer.class).in(Scopes.SINGLETON);

newOptionalBinder(binder(), ProxyResponseTransformer.class).setBinding()

.to(ProxyResponseTransformer.class)

.in(Scopes.SINGLETON);

or we can make ProxyResponseTransformer into an interface and have different impl...

ebyhr · 2025-06-17T04:51:35Z

@cla-bot check

cla-bot · 2025-06-17T04:51:39Z

The cla-bot has been summoned, and re-checked this pull request!

andythsu · 2025-06-24T23:31:57Z

Please resolve merge conflict @yaelselig30

ebyhr reviewed May 22, 2025

View reviewed changes

yaelselig30 force-pushed the yael/refactor-proxy-request-handler branch from 9a921d9 to 21a7937 Compare May 25, 2025 08:18

yaelselig30 force-pushed the yael/refactor-proxy-request-handler branch from 21a7937 to 2fadbb7 Compare May 25, 2025 08:34

yaelselig30 requested a review from ebyhr May 25, 2025 08:55

yaelselig30 commented May 27, 2025

View reviewed changes

andythsu suggested changes May 27, 2025

View reviewed changes

ebyhr requested a review from Copilot June 2, 2025 07:44

Copilot AI reviewed Jun 2, 2025

View reviewed changes

Extract Future transformation logic into dedicated class

79c9b5d

This change moves the transformation logic applied to Future instances into a separate class, making it easier to extend and maintain. It also helps clarify the responsibilities of each component involved.

yaelselig30 force-pushed the yael/refactor-proxy-request-handler branch from 2fadbb7 to 79c9b5d Compare June 3, 2025 14:18

andythsu reviewed Jun 5, 2025

View reviewed changes

cla-bot bot added the cla-signed label Jun 17, 2025

		public ProxyResponseTransformer(QueryHistoryManager queryHistoryManager,
		RoutingManager routingManager)

		this.routingManager = requireNonNull(routingManager, "routingManager is null");
		this.queryHistoryManager = requireNonNull(queryHistoryManager, "queryHistoryManager is null");

		private static final Logger log = Logger.get(ProxyRequestHandler.class);
		private final QueryHistoryManager queryHistoryManager;

		catch (IOException e) {
		log.error("Failed to get QueryId from response [%s] , Status code [%s]", response.body(), response.statusCode());

	if (Strings.isNullOrEmpty(username)) {
	if (!Strings.isNullOrEmpty(username)) {

Extracts the transformation logic on Future instances into a dedicated class #688

Are you sure you want to change the base?

Extracts the transformation logic on Future instances into a dedicated class #688

Uh oh!

Conversation

yaelselig30 commented May 22, 2025 • edited by ebyhr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

cla-bot bot commented May 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr commented May 22, 2025

Uh oh!

cla-bot bot commented May 25, 2025

Uh oh!

cla-bot bot commented May 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Jun 17, 2025

Uh oh!

cla-bot bot commented Jun 17, 2025

Uh oh!

andythsu commented Jun 24, 2025

Uh oh!

Uh oh!

yaelselig30 commented May 22, 2025 •

edited by ebyhr

Loading