[FLINK-13368][python] Add Configuration Class for Python Table API to Align with Java. by WeiZhong94 · Pull Request #9199 · apache/flink

WeiZhong94 · 2019-07-22T12:38:25Z

What is the purpose of the change

This pull request adds Configuration class, TableConfig.get_configuration() method and TableConfig.add_configuration() method for python Table API to align with Java.

Brief change log

Add Configuration class and relevant tests in pyflink.common package
Add get_configuration() and add_configuration() methods in TableConfig class
Extract the tests of TableConfig into a separate file

Verifying this change

This change is already covered by existing tests, such as test_configuration.py and test_table_config.py.

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): (no)
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? (python docs)

… Align with Java.

flinkbot · 2019-07-22T12:41:10Z

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 ffc1258 (Tue Aug 06 15:45:25 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.
- Approved by @sunjincheng121 [committer]
✅ 2. There is [consensus] that the contribution should go into to Flink.
- Approved by @sunjincheng121 [committer]
❓ 3. Needs [attention] from.
✅ 4. The change fits into the overall [architecture].
- Approved by @sunjincheng121 [committer]
✅ 5. Overall code [quality] is good.
- Approved by @sunjincheng121 [committer]

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-07-22T12:47:45Z

CI report:

39f1f29 : FAILURE Build
f238bca : FAILURE Build
ffc1258 : FAILURE Build

sunjincheng121

Thanks for the PR @WeiZhong94 !
I think It's better to merge the test methods just like java test case.
Best, Jincheng

sunjincheng121 · 2019-07-22T13:09:13Z

flink-python/pyflink/common/tests/test_configuration.py

+from pyflink.testing.test_case_utils import PyFlinkTestCase
+
+
+class ConfigurationTests(PyFlinkTestCase):


Could you please merge the testcase just like Java, please see: ConfigurationTest

dianfu

@WeiZhong94 Thanks a lot for the PR. Have left a few minor comments.

dianfu · 2019-07-23T01:04:04Z

flink-python/pyflink/common/configuration.py

+        else:
+            self._j_configuration.addAll(other._j_configuration, prefix)
+
+    def clone(self):


should use copy and deepcopy instead.

dianfu · 2019-07-23T01:05:48Z

flink-python/pyflink/common/configuration.py

+
+    def to_dict(self):
+        """
+        Convert the configuration into a dict representation of string key-pair.


Convert -> Converts

WeiZhong94 · 2019-07-23T01:57:16Z

@sunjincheng121 Thanks a lot for your review! I have combined these test cases into one in the latest commit.

WeiZhong94 · 2019-07-23T02:42:12Z

@dianfu Thanks a lot for your review! I have addressed your comments.

dianfu · 2019-07-23T02:44:03Z

@WeiZhong94 Thanks a lot for the update. LGTM to me. +1 from my side.

sunjincheng121

+1 to merged 👍
@flinkbot approve all

… Align with Java. This closes #9199

[FLINK-13368][python] Add Configuration Class for Python Table API to…

39f1f29

… Align with Java.

rmetzger added the review=description? label Jul 22, 2019

sunjincheng121 reviewed Jul 22, 2019

View reviewed changes

rmetzger added the component=API/Python label Jul 22, 2019

dianfu reviewed Jul 23, 2019

View reviewed changes

combine getter and setter test cases into one test case.

f238bca

Rename clone method to "__deepcopy__" and fix docs typo.

ffc1258

sunjincheng121 approved these changes Jul 23, 2019

View reviewed changes

asfgit closed this in 2d8ba48 Jul 23, 2019

asfgit pushed a commit that referenced this pull request Jul 23, 2019

[FLINK-13368][python] Add Configuration Class for Python Table API to…

39f4fc2

… Align with Java. This closes #9199

rmetzger added review=approved ✅ and removed review=description? labels Aug 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-13368][python] Add Configuration Class for Python Table API to Align with Java.#9199

[FLINK-13368][python] Add Configuration Class for Python Table API to Align with Java.#9199
WeiZhong94 wants to merge 3 commits intoapache:masterfrom
WeiZhong94:FLINK-13368

WeiZhong94 commented Jul 22, 2019

Uh oh!

flinkbot commented Jul 22, 2019 •

edited

Loading

Uh oh!

flinkbot commented Jul 22, 2019 •

edited

Loading

Uh oh!

sunjincheng121 left a comment

Uh oh!

sunjincheng121 Jul 22, 2019

Uh oh!

dianfu left a comment

Uh oh!

dianfu Jul 23, 2019

Uh oh!

dianfu Jul 23, 2019

Uh oh!

WeiZhong94 commented Jul 23, 2019

Uh oh!

WeiZhong94 commented Jul 23, 2019

Uh oh!

dianfu commented Jul 23, 2019

Uh oh!

sunjincheng121 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		from pyflink.testing.test_case_utils import PyFlinkTestCase


		class ConfigurationTests(PyFlinkTestCase):

Conversation

WeiZhong94 commented Jul 22, 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 Jul 22, 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 Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

sunjincheng121 left a comment

Choose a reason for hiding this comment

Uh oh!

sunjincheng121 Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

dianfu left a comment

Choose a reason for hiding this comment

Uh oh!

dianfu Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

dianfu Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

WeiZhong94 commented Jul 23, 2019

Uh oh!

WeiZhong94 commented Jul 23, 2019

Uh oh!

dianfu commented Jul 23, 2019

Uh oh!

sunjincheng121 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flinkbot commented Jul 22, 2019 •

edited

Loading

flinkbot commented Jul 22, 2019 •

edited

Loading