[FLINK-13600][table] Rework TableEnvironment.connect() class hierarchy #9382

twalthr · 2019-08-07T14:11:56Z

What is the purpose of the change

This fixes the currently broken TableEnvironment.connect() API by reworking the descriptor class hierarchy. TableDescriptor now contains most of the logic and ConnectTableDescriptor is the return type of table environments (expect for legacy Batch/StreamTableDescriptors).

The API is still mostly source code compatible.

Brief change log

Return ConnectTableDescriptor
Merge ConnectorFormatDescriptor, StreamableDescriptor, and TableDescriptor
Merge RegistrableDescriptor and ConnectTableDescriptor

Verifying this change

A new TableEnvironmentTest tests the implementation.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): yes
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? JavaDocs

flinkbot · 2019-08-07T14:14:36Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 763fa08 (Thu Aug 08 15:38:33 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-08-07T14:23:48Z

CI report:

8c8402a : FAILURE Build
02aabb9 : FAILURE Build
763fa08 : FAILURE Build

dawidwys

I think this is a good step to make the TableDescriptors simpler.

As we are reworking the hierarchy I think it makes sense to fix the contract of TableDescriptor#toProperties & ConnectTableDescriptor#toProperties as part of this.

dawidwys · 2019-08-08T07:54:59Z

...e/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogTableBuilder.java

-	private Optional<FormatDescriptor> formatDescriptor = Optional.empty();
+	private final boolean isGeneric;
+
 	private Optional<Statistics> statisticsDescriptor = Optional.empty();


I know this would be unrelated change, but how about we drop the statisticsDescriptor & metadataDescriptor. They cannot be set, they are always empty, so they are effectively useless. Moreover they use Optional as a class field which is against our code style guidelines.

dawidwys · 2019-08-08T08:01:30Z

...-table-api-java/src/main/java/org/apache/flink/table/descriptors/ConnectTableDescriptor.java

 	 * Converts this descriptor into a set of properties.
 	 */
 	@Override
 	public Map<String, String> toProperties() {


How about we make this method a bit safer to extend. Right now it adds implicit contract that you have to call super.toProperties in a child class.

Maybe we could change it to:

public final Map<String, String> toProperties() { ... properties.asMap().putAll(extraProperties()); } protected abstract Map<String, String> extraProperties();

I just saw it's done this way already for FormatDescriptor

dawidwys · 2019-08-08T08:07:39Z

...k-table-api-java/src/main/java/org/apache/flink/table/descriptors/StreamTableDescriptor.java

-	implements StreamableDescriptor<StreamTableDescriptor> {
-
-	private Optional<String> updateMode = Optional.empty();
+public final class StreamTableDescriptor extends ConnectTableDescriptor {


Probably we can't do it because of backwards compatibility, but I will put it here anyway.

Do we even need the StreamTableDescriptor? It has no additional methods compared to ConnectTableDescriptor

Backwards compatibility is the answer. I will add a comment to the class.

dawidwys · 2019-08-08T08:26:24Z

...le/flink-table-api-java/src/test/java/org/apache/flink/table/utils/TableEnvironmentMock.java

+		this.planner = planner;
+	}
+
+	public static TableEnvironmentMock getInstance(boolean isStreamingMode) {


nit: getInstance(Boolean) -> getStreamingInstance + getBatchInstance

dawidwys · 2019-08-08T08:37:10Z

...ble/flink-table-common/src/main/java/org/apache/flink/table/descriptors/TableDescriptor.java

+ */
 @PublicEvolving
-public abstract class TableDescriptor extends DescriptorBase {
+public abstract class TableDescriptor<D extends TableDescriptor<D>> extends DescriptorBase {


Do we need both TableDescriptor & ConnectTableDescriptor?

There is only a single descriptor that extends from TableDescriptor directly (and to be honest I couldn't find any reasonable usage of it in a production code). Moreover I think the sole purpose of a descriptor is to register it somewhere. Isn't it?

TableDescriptor is used in CatalogTableBuilder. The main reason of this class is the generic that is not present in ConnectTableDescriptor.

dawidwys · 2019-08-08T08:37:40Z

...ble/flink-table-common/src/main/java/org/apache/flink/table/descriptors/TableDescriptor.java

+	 * Converts this descriptor into a set of properties.
+	 */
+	@Override
+	public Map<String, String> toProperties() {


The same problem as for ConnectTableDescriptor. Adds implicit contract.

dawidwys

Thanks for the update @twalthr. I think it's good to be merged now.

twalthr · 2019-08-08T13:09:18Z

Merging...

This closes #9382.

This closes apache#9382.

[FLINK-13600][table] Rework TableEnvironment.connect() class hierarchy

8c8402a

rmetzger added the review=description? label Aug 7, 2019

rmetzger added the component=TableSQL/API label Aug 7, 2019

dawidwys requested changes Aug 8, 2019

View reviewed changes

Feedback addressed

763fa08

twalthr force-pushed the FLINK-13600 branch from 02aabb9 to 763fa08 Compare August 8, 2019 11:27

dawidwys approved these changes Aug 8, 2019

View reviewed changes

asfgit pushed a commit that referenced this pull request Aug 8, 2019

[FLINK-13600][table] Rework TableEnvironment.connect() class hierarchy

88458eb

This closes #9382.

asfgit closed this in d544d30 Aug 8, 2019

becketqin pushed a commit to becketqin/flink that referenced this pull request Aug 17, 2019

[FLINK-13600][table] Rework TableEnvironment.connect() class hierarchy

c434591

This closes apache#9382.

[FLINK-13600][table] Rework TableEnvironment.connect() class hierarchy #9382

[FLINK-13600][table] Rework TableEnvironment.connect() class hierarchy #9382

Uh oh!

Conversation

twalthr commented Aug 7, 2019

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

flinkbot commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Checks

Review Progress

Uh oh!

flinkbot commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

dawidwys left a comment

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!

dawidwys Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dawidwys left a comment

Choose a reason for hiding this comment

Uh oh!

twalthr commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flinkbot commented Aug 7, 2019 •

edited

Loading

flinkbot commented Aug 7, 2019 •

edited

Loading

dawidwys Aug 8, 2019 •

edited

Loading