[FLINK-27767][sql-gateway] Introduce Endpoint API and utils #19849
Conversation
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| /** Test for {@link SessionManager}. */ | ||
| public class SessionManagerTest { |
There was a problem hiding this comment.
A small suggestion: I think we should now use Junit5 and assertj in newly introduced tests.
There was a problem hiding this comment.
Thanks for your suggestions. I use Junit5 in the current implementation.
fsk119
force-pushed
the
endpoint-api
branch
2 times, most recently
from
eb3d4a1
to
9c59167
Compare
...rc/main/java/org/apache/flink/table/gateway/api/endpoint/SqlGatewayEndpointFactoryUtils.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/flink/table/gateway/api/endpoint/SqlGatewayEndpointFactoryUtils.java
Outdated
Show resolved
Hide resolved
| SqlGatewayEndpointFactory.class, | ||
| identifier); | ||
| Configuration endpointConfig = | ||
| new DelegatingConfiguration( |
There was a problem hiding this comment.
Shall we need to delegate all configurations with the endpoint prefix? This disallows the endpoint to access session configurations, such as options in SqlGatewayServiceConfigOptions which might be helpful in some cases.
There was a problem hiding this comment.
Yes. Currently we have getFlinkConfiguration and getEndpointOptionsnow. Users are able to get SqlGatewayService config from getFlinkConfiguration.
| public ReadableConfig getConfiguration() { | ||
| return configuration; | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, String> getOptions() { | ||
| return configuration.toMap(); | ||
| } |
There was a problem hiding this comment.
This looks like a design flaw that if getConfiguration and getOptions are equivalent, then why do we need to provide both of them?
In DynamicTableFactory, the configuration represents the configs of the session, and options represent the table WITH options. I think we can change the methods to getFlinkConfiguration() and getEndpointOptions() to make it clear.
.../src/test/java/org/apache/flink/table/gateway/api/utils/MockedSqlGatewayEndpointFactory.java
Outdated
Show resolved
Hide resolved
|
@flinkbot run azure |
|
@flinkbot run azure |
What is the purpose of the change
Introdue Endpoint API and utils to create endpoint.
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation