Use appropriate connector instance based on protocol

[prakhar10]

This is to fix #242

[prakhar10]

@mosabua @ebyhr I'm not really sure how such cases are being handled in Trino repo, but i can take a look. If there are any suggestions please let me know.

[mosabua]

Can you fix the test as requested @prakhar10

[prakhar10]

Can you fix the test as requested @prakhar10

Sorry for the delay, got busy with something else. I'll fix this in a couple of days.

[ebyhr]

The implementation of getPort() method looks same between HttpsConnectorFactory and HttpConnectorFactory. Why do we need to differentiate them?

[mosabua]

I looked at porting this to the new class now and also this code. Honestly I share the questions from @ebyhr and I am not sure at all why this change would do anything. I looked the the source of the two factory classes as well and the https one just extends the http one and there is no modification or change or so for the port.

So if this change really resolves the issues.. I dont know why. Thoughts @prakhar10 ?

[ebyhr]

We should ensure the retrieved port is unique. The subsequent findFirst() & orElseThrow() needs fix.

[mosabua]

Should we just use the changes from #294 with the test from this PR and improve in this PR as next step? Could you do that @prakhar10 ?

[prakhar10]

Should we just use the changes from #294 with the test from this PR and improve in this PR as next step? Could you do that @prakhar10 ?

Sure @mosabua , but I believe we started off with a similar change and with reviews we've come to this code that I have currently in this PR.

But if it helps and if everyone agrees I can bring in the code changes from that PR into this one and we can take it forward.

[mosabua]

Should we just use the changes from #294 with the test from this PR and improve in this PR as next step? Could you do that @prakhar10 ?

Sure @mosabua , but I believe we started off with a similar change and with reviews we've come to this code that I have currently in this PR.

But if it helps and if everyone agrees I can bring in the code changes from that PR into this one and we can take it forward.

Ideally we just address the outstanding comments from @ebyhr ... and ensure the test works as expected. I really think we need to get this merged for the next release.

[ebyhr]

By the way, test isn't mandatory in my opinion. This is pretty small change and will be removed when migrating to Airlift.

[ebyhr]

Could you rebase on main to resolve conflicts?

[prakhar10]

Could you rebase on main to resolve conflicts?

Looks like this file does not exist anymore in the main branch, do we need this PR then @ebyhr ?

[ebyhr]

@prakhar10 Please take a look at HaGatewayProviderModule:

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

Lines 168 to 183 in 3c20299

	private int getApplicationPort()
	{
	Stream<ConnectorFactory> connectors =
	configuration.getServerFactory() instanceof DefaultServerFactory
	? ((DefaultServerFactory) configuration.getServerFactory())
	.getApplicationConnectors().stream()
	: Stream.of((SimpleServerFactory) configuration.getServerFactory())
	.map(SimpleServerFactory::getConnector);
	return connectors
	.filter(connector -> connector.getClass().isAssignableFrom(HttpConnectorFactory.class))
	.map(connector -> (HttpConnectorFactory) connector)
	.mapToInt(HttpConnectorFactory::getPort)
	.findFirst()
	.orElseThrow(IllegalStateException::new);
	}

[mosabua]

@prakhar10 are you going to move this change over to the new module? Even potentially without test .. just so we can get this merged and maybe ship as part of the 8 release?

[willmostly]

Hi @prakhar10 , do you need any assistance with this?