You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@inlong.apache.org by "bibhu107 (via GitHub)" <gi...@apache.org> on 2023/04/16 10:48:07 UTC
[GitHub] [inlong] bibhu107 opened a new pull request, #7864: Fixing issue #6668
bibhu107 opened a new pull request, #7864:
URL: https://github.com/apache/inlong/pull/7864
### Prepare a Pull Request
*(Change the title refer to the following example)*
Removing redundant hard coded constants.
- Title:[INLONG-SORT] Removing hard codings and using static constants
- Fixes #6668
### Motivation
*Explain here the context, and why you're making that change. What is the problem you're trying to solve?*
Using already existing constants and removing hard codings
### Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) not documented
- If a feature is not applicable for documentation, explain why? This change needs no documentation as it's using already created constants at hard-coded strings
- If a feature is not documented yet in this PR, please create a follow-up issue for adding the documentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gong commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "gong (via GitHub)" <gi...@apache.org>.
gong commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1168541323
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/util/FlinkConfiguration.java:
##########
@@ -29,7 +29,7 @@
import static org.apache.inlong.manager.plugin.flink.enums.Constants.ADDRESS;
import static org.apache.inlong.manager.plugin.flink.enums.Constants.JOB_MANAGER_PORT;
-import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
Why don't you use `org.apache.inlong.common.constant.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] bibhu107 commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "bibhu107 (via GitHub)" <gi...@apache.org>.
bibhu107 commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1169524857
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/flink/enums/Constants.java:
##########
@@ -40,7 +40,7 @@ public class Constants {
public static final String DRAIN = "flink.drain";
- public static final String METRICS_AUDIT_PROXY_HOSTS = "metrics.audit.proxy.hosts";
+ public static final String METRICS_AUDIT_PROXY_HOSTS_KEY = "metrics.audit.proxy.hosts";
Review Comment:
Hi @healchow
Thanks for the suggestions. Have taken care of these in new commit. Please check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] bibhu107 commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "bibhu107 (via GitHub)" <gi...@apache.org>.
bibhu107 commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1169525340
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/util/FlinkConfiguration.java:
##########
@@ -29,7 +29,7 @@
import static org.apache.inlong.manager.plugin.flink.enums.Constants.ADDRESS;
import static org.apache.inlong.manager.plugin.flink.enums.Constants.JOB_MANAGER_PORT;
-import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
Hi @gong
Have considered the above comments and raised a new commit. Please check and let me know if it looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "healchow (via GitHub)" <gi...@apache.org>.
healchow commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1168096956
##########
inlong-sort/sort-connectors/base/src/main/java/org/apache/inlong/sort/base/Constants.java:
##########
@@ -20,6 +20,8 @@
import org.apache.flink.configuration.ConfigOption;
import org.apache.flink.configuration.ConfigOptions;
import org.apache.inlong.sort.base.sink.SchemaUpdateExceptionPolicy;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
Hi, the `inlong-sort` module should not dependency the `inlong-manager` module directly, can you move the `org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY` to `inlong-common` module?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gong commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "gong (via GitHub)" <gi...@apache.org>.
gong commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1168541323
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/util/FlinkConfiguration.java:
##########
@@ -29,7 +29,7 @@
import static org.apache.inlong.manager.plugin.flink.enums.Constants.ADDRESS;
import static org.apache.inlong.manager.plugin.flink.enums.Constants.JOB_MANAGER_PORT;
-import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
@bibhu107 Hi, Why don't you use `org.apache.inlong.common.constant.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] dockerzhang commented on pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "dockerzhang (via GitHub)" <gi...@apache.org>.
dockerzhang commented on PR #7864:
URL: https://github.com/apache/inlong/pull/7864#issuecomment-1510578721
@bibhu107 You could run `mvn spotless:apply` to fix the check style error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] bibhu107 commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "bibhu107 (via GitHub)" <gi...@apache.org>.
bibhu107 commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1168204880
##########
inlong-sort/sort-connectors/base/src/main/java/org/apache/inlong/sort/base/Constants.java:
##########
@@ -20,6 +20,8 @@
import org.apache.flink.configuration.ConfigOption;
import org.apache.flink.configuration.ConfigOptions;
import org.apache.inlong.sort.base.sink.SchemaUpdateExceptionPolicy;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
Hi @healchow
Have taken care of this in next commit. Please let me know if the changes look good?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gong commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "gong (via GitHub)" <gi...@apache.org>.
gong commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1168541323
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/util/FlinkConfiguration.java:
##########
@@ -29,7 +29,7 @@
import static org.apache.inlong.manager.plugin.flink.enums.Constants.ADDRESS;
import static org.apache.inlong.manager.plugin.flink.enums.Constants.JOB_MANAGER_PORT;
-import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
@bibhu107 Hi, Why don't you reuse `org.apache.inlong.common.constant.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] dockerzhang merged pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "dockerzhang (via GitHub)" <gi...@apache.org>.
dockerzhang merged PR #7864:
URL: https://github.com/apache/inlong/pull/7864
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "healchow (via GitHub)" <gi...@apache.org>.
healchow commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1168568247
##########
inlong-sort/sort-connectors/base/src/main/java/org/apache/inlong/sort/base/Constants.java:
##########
@@ -20,6 +20,8 @@
import org.apache.flink.configuration.ConfigOption;
import org.apache.flink.configuration.ConfigOptions;
import org.apache.inlong.sort.base.sink.SchemaUpdateExceptionPolicy;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "healchow (via GitHub)" <gi...@apache.org>.
healchow commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1169425854
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/flink/enums/Constants.java:
##########
@@ -40,7 +40,7 @@ public class Constants {
public static final String DRAIN = "flink.drain";
- public static final String METRICS_AUDIT_PROXY_HOSTS = "metrics.audit.proxy.hosts";
+ public static final String METRICS_AUDIT_PROXY_HOSTS_KEY = "metrics.audit.proxy.hosts";
Review Comment:
Can we remove this constant, and using the same constant from the `inlong-common` module?
I verified that the `manager-plugins` module has already dependent the `inlong-common` module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gong commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "gong (via GitHub)" <gi...@apache.org>.
gong commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1169425626
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/util/FlinkConfiguration.java:
##########
@@ -29,7 +29,7 @@
import static org.apache.inlong.manager.plugin.flink.enums.Constants.ADDRESS;
import static org.apache.inlong.manager.plugin.flink.enums.Constants.JOB_MANAGER_PORT;
-import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
> Hi @gong I tried adding `org.apache.inlong.common.constant.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY` to `org.apache.inlong.common.constant.Constants` and also directly using in `inlong-sort` but in both the options were needing to create dependency on 'manager-plugins' which is not correct which might create circular dependency between modules 'inlong-common' and 'manager-common'. For reference you can check commit-1
@bibhu107 Hi, I think you can `inlong-sort` and `manager-plugin` module dependency `inlong-common` after adding `org.apache.inlong.common.constant.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY` to `org.apache.inlong.common.constant.Constants`. `inlong-sort` module don't need to dependency `manager-plugin` module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gong commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "gong (via GitHub)" <gi...@apache.org>.
gong commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1169729781
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/util/FlinkConfiguration.java:
##########
@@ -29,7 +29,7 @@
import static org.apache.inlong.manager.plugin.flink.enums.Constants.ADDRESS;
import static org.apache.inlong.manager.plugin.flink.enums.Constants.JOB_MANAGER_PORT;
-import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
> Hi @gong
>
> Have considered the above comments and raised a new commit. Please check and let me know if it looks good.
look good to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "healchow (via GitHub)" <gi...@apache.org>.
healchow commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1169725443
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/flink/enums/Constants.java:
##########
@@ -40,7 +40,7 @@ public class Constants {
public static final String DRAIN = "flink.drain";
- public static final String METRICS_AUDIT_PROXY_HOSTS = "metrics.audit.proxy.hosts";
+ public static final String METRICS_AUDIT_PROXY_HOSTS_KEY = "metrics.audit.proxy.hosts";
Review Comment:
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] bibhu107 commented on a diff in pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "bibhu107 (via GitHub)" <gi...@apache.org>.
bibhu107 commented on code in PR #7864:
URL: https://github.com/apache/inlong/pull/7864#discussion_r1168595514
##########
inlong-manager/manager-plugins/src/main/java/org/apache/inlong/manager/plugin/util/FlinkConfiguration.java:
##########
@@ -29,7 +29,7 @@
import static org.apache.inlong.manager.plugin.flink.enums.Constants.ADDRESS;
import static org.apache.inlong.manager.plugin.flink.enums.Constants.JOB_MANAGER_PORT;
-import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS;
+import static org.apache.inlong.manager.plugin.flink.enums.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY;
Review Comment:
Hi @gong
I tried adding `org.apache.inlong.common.constant.Constants.METRICS_AUDIT_PROXY_HOSTS_KEY` to `org.apache.inlong.common.constant.Constants` and also directly using in `inlong-sort` but in both the options were needing to create dependency on 'manager-plugins' which is not correct which might create circular dependency between modules 'inlong-common' and 'manager-common'.
For reference you can check commit-1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] bibhu107 commented on pull request #7864: [INLONG-6668][Sort] Reusing existing constants
Posted by "bibhu107 (via GitHub)" <gi...@apache.org>.
bibhu107 commented on PR #7864:
URL: https://github.com/apache/inlong/pull/7864#issuecomment-1510746099
> @bibhu107 You could run `mvn spotless:apply` to fix the check style error.
With second commit ran `mvn spotless:apply`, it built successfully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org