Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature][Core] Rename result_table_name/source_table_name to plugin_input/plugin_output #8072

Merged
merged 5 commits into from
Nov 23, 2024

Conversation

hailin0
Copy link
Member

@hailin0 hailin0 commented Nov 16, 2024

Purpose of this pull request

close #8062

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@hailin0 hailin0 closed this Nov 16, 2024
@hailin0 hailin0 reopened this Nov 16, 2024
@hailin0 hailin0 marked this pull request as draft November 16, 2024 05:29
@hailin0 hailin0 force-pushed the dev-rename-common-config branch 2 times, most recently from 6df5ac4 to 1eb8e16 Compare November 16, 2024 05:41
@hailin0 hailin0 marked this pull request as ready for review November 17, 2024 08:19
@Hisoka-X Hisoka-X changed the title [Refactor][Config] Rename result_table_name/source_table_name [Feature][Core] Rename result_table_name/source_table_name to plugin_input/plugin_output Nov 18, 2024
@hailin0 hailin0 force-pushed the dev-rename-common-config branch from 1eb8e16 to 9025604 Compare November 18, 2024 01:50
@Hisoka-X
Copy link
Member

This is a big change. cc @TyrantLucifer @liugddx @liunaijie @dailai @Carl-Zhou-CN

dailai
dailai previously approved these changes Nov 18, 2024
Copy link
Contributor

@dailai dailai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@liugddx
Copy link
Member

liugddx commented Nov 19, 2024

Will this affect existing tasks?

.stringType()
.noDefaultValue()
.withFallbackKeys("result_table_name")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compatible with old versions

cc @liugddx

@Carl-Zhou-CN
Copy link
Member

@hailin0 Do we need to keep a few test cases and end-to-end tests for backward compatibility with the old version?

@hailin0 hailin0 force-pushed the dev-rename-common-config branch from 45f62cc to 7e31e2a Compare November 19, 2024 16:53
@hailin0 hailin0 force-pushed the dev-rename-common-config branch from 7e31e2a to 22405d4 Compare November 20, 2024 02:27
@hailin0
Copy link
Member Author

hailin0 commented Nov 20, 2024

PTAL

dailai
dailai previously approved these changes Nov 21, 2024
liunaijie
liunaijie previously approved these changes Nov 21, 2024
@liunaijie
Copy link
Member

liunaijie commented Nov 21, 2024

This is a big change, even it's backward compatibility with the old version, but how many version should we keep backward, all the time or like 2 or 3 versions?

How about add a upgrade note on each release version, in this page, introduce how to upgrade from old version (which is the minimum version). which feature (parameters) is deprecated, moreover we can introduce the features, fixed bug.

Now we have the release-node, but it's not enough.

WDYT

@Hisoka-X
Copy link
Member

how many version should we keep backward, all the time or like 2 or 3 versions

I think we should keep it indefinitely, not just for 2 or 3 versions. Perhaps after one or two years, we can revisit it to decide whether to keep or remove it, as keeping it poses no burden to us. However, we should mention in the documentation that the use of result_table_name and source_table_name is no longer recommended. cc @hailin0

How about add a upgrade note on each release version, in this page, introduce how to upgrade from old version (which is the minimum version). which feature (parameters) is deprecated, moreover we can introduce the features, fixed bug.

+1

@Carl-Zhou-CN
Copy link
Member

how many version should we keep backward, all the time or like 2 or 3 versions

I think we should keep it indefinitely, not just for 2 or 3 versions. Perhaps after one or two years, we can revisit it to decide whether to keep or remove it, as keeping it poses no burden to us. However, we should mention in the documentation that the use of result_table_name and source_table_name is no longer recommended. cc @hailin0

How about add a upgrade note on each release version, in this page, introduce how to upgrade from old version (which is the minimum version). which feature (parameters) is deprecated, moreover we can introduce the features, fixed bug.

+1

+1

@hailin0
Copy link
Member Author

hailin0 commented Nov 21, 2024

This is a big change, even it's backward compatibility with the old version, but how many version should we keep backward, all the time or like 2 or 3 versions?

How about add a upgrade note on each release version, in this page, introduce how to upgrade from old version (which is the minimum version). which feature (parameters) is deprecated, moreover we can introduce the features, fixed bug.

Now we have the release-node, but it's not enough.

WDYT

Yes, this is what is still missing

@hailin0
Copy link
Member Author

hailin0 commented Nov 21, 2024

how many version should we keep backward, all the time or like 2 or 3 versions

I think we should keep it indefinitely, not just for 2 or 3 versions. Perhaps after one or two years, we can revisit it to decide whether to keep or remove it, as keeping it poses no burden to us. However, we should mention in the documentation that the use of result_table_name and source_table_name is no longer recommended. cc @hailin0

How about add a upgrade note on each release version, in this page, introduce how to upgrade from old version (which is the minimum version). which feature (parameters) is deprecated, moreover we can introduce the features, fixed bug.

+1

@Hisoka-X
Current, an outdated parameter warning will be printed in the log.

@Hisoka-X
Copy link
Member

Current, an outdated parameter warning will be printed in the log.

We should metion it in doc too, let users to consciously change expired configurations. Doc more important than log.

@hailin0
Copy link
Member Author

hailin0 commented Nov 22, 2024

Current, an outdated parameter warning will be printed in the log.

We should metion it in doc too, let users to consciously change expired configurations. Doc more important than log.

Added notes.

@hailin0 hailin0 merged commit c7bbd32 into apache:dev Nov 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][Config] Rename source_table_name /result_table_name
6 participants