You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "nateab (via GitHub)" <gi...@apache.org> on 2023/04/05 20:55:33 UTC
[GitHub] [flink] nateab opened a new pull request, #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
nateab opened a new pull request, #22359:
URL: https://github.com/apache/flink/pull/22359
## What is the purpose of the change
This PR adds the `json-path` dependency to the table-planner pom so that `ITCases` in the kafka connector module can run in the Intellij IDE. This is due to an old Intellij bug that does not handle the maven-shade plugin correctly https://youtrack.jetbrains.com/issue/IDEA-93855.
## Brief change log
- Add the `json-path` to the `flink-table-planner_${scala.binary.version}` pom
## Verifying this change
This change is already covered by existing tests, such as `KafkaChangelogTableITCase`.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): yes
- 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, Kubernetes/Yarn, 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? (not applicable / docs / JavaDocs / not documented)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on PR #22359:
URL: https://github.com/apache/flink/pull/22359#issuecomment-1527980162
CI was failing upstream because of an outdated library, which was fixed in this commit https://github.com/apache/flink/commit/ed7ca22efc68203d4882887a3856434137e6980f, gonna rerun the CI
@flinkbot run azure
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1178509636
##########
flink-table/flink-table-planner-loader/pom.xml:
##########
@@ -44,13 +44,11 @@
<artifactId>flink-table-api-bridge-base</artifactId>
<version>${project.version}</version>
</dependency>
-
<dependency>
<!-- Test utils -->
<groupId>org.apache.flink</groupId>
<artifactId>flink-test-utils-junit</artifactId>
</dependency>
-
Review Comment:
reverted
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on PR #22359:
URL: https://github.com/apache/flink/pull/22359#issuecomment-1530190208
@zentol I have changed the version to be 2.4.0 and updated the notice file so that CI passes, could you take another look please
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1184383181
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,11 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>2.4.0</version>
+ </dependency>
Review Comment:
I have removed jayway from the `PlannerModule#KNOWN_MODULE_ASSOCIATIONS` and removed the explicit dependency in the planner, and added a dependency entry in `flink-table`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1162460793
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,13 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>${jsonpath.version}</version>
+ <scope>test</scope>
Review Comment:
Add an entry to the dependencyManagement of the flink-table module for json-path.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on PR #22359:
URL: https://github.com/apache/flink/pull/22359#issuecomment-1538424658
Oh wow it did actually merge it before but failed to close the PR :facepalm:
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on PR #22359:
URL: https://github.com/apache/flink/pull/22359#issuecomment-1526076794
CI failed with this, will look at fixing the notice file
```01:53:59,901 INFO org.apache.flink.tools.ci.licensecheck.NoticeFileChecker [] - Problems were detected for a NOTICE file.
flink-table-planner:
These issue are legally problematic and MUST be fixed:
Dependency com.jayway.jsonpath:json-path:2.4.0 is not listed.```
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1160403337
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,13 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>${jsonpath.version}</version>
+ <scope>test</scope>
Review Comment:
Nvm that wasn't it, i had to add `<include>com.jayway.jsonpath:json-path</include>` to get it to work
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on PR #22359:
URL: https://github.com/apache/flink/pull/22359#issuecomment-1524274234
@flinkbot run azure
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1183893561
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -392,6 +397,8 @@ under the License.
<!-- flink-table-planner dependencies -->
<include>org.apache.flink:flink-sql-parser</include>
<include>org.apache.flink:flink-sql-parser-hive</include>
+
+ <include>com.jayway.jsonpath:json-path</include>
Review Comment:
The comment below about the jayway relocation ("\<!-- From flink-table-runtime -->") is now out-dated. The relocation should also be moved up to the calcite dependencies block, and this entry be moved up to the calcite dependency block.
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,11 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>2.4.0</version>
+ </dependency>
Review Comment:
You now have to dependencies on json-path (table-planner/-runtime respectively), both defining the version but using the same relocation pattern, which could lead to conflicts at runtime.
You want to remove jayway from the `PlannerModule#KNOWN_MODULE_ASSOCIATIONS` so the planner doesn't have access to this dependency from the table-runtime.
I would suggest to add a dependencyManagement to `flink-table` to control the version (and then remove the explicit dependency here and just pull it in via calcite).
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1160430061
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,13 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>${jsonpath.version}</version>
+ <scope>test</scope>
Review Comment:
CI failed with
```
Dependency convergence error for com.jayway.jsonpath:json-path:jar:2.4.0:runtime paths to dependency are:
+-org.apache.flink:flink-table-planner_2.12:jar:1.17-SNAPSHOT
+-org.apache.calcite:calcite-core:jar:1.29.0:compile
+-com.jayway.jsonpath:json-path:jar:2.4.0:runtime
and
+-org.apache.flink:flink-table-planner_2.12:jar:1.17-SNAPSHOT
+-com.jayway.jsonpath:json-path:jar:2.6.0:compile
```
maybe i need to try adding the dependency in the `flink-table-planner-loader` pom instead?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1162463096
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,13 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>${jsonpath.version}</version>
+ <scope>test</scope>
Review Comment:
_or_ use json-path 2.4.0 (because that's what calcite expects anyway). In fact, we don't need the additional dependency in the pom; the include alone would solve this.
We should use a different relocation pattern though to ensure this doesn't clash with the json-path dependency of the table runtime.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on PR #22359:
URL: https://github.com/apache/flink/pull/22359#issuecomment-1535370028
@zentol i believe i've addressed all your feedback, and I got CI to be pass again, so if you could take a final look hopefully 🤞 And thanks for all your help reviewing!
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol closed pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol closed pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
URL: https://github.com/apache/flink/pull/22359
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on PR #22359:
URL: https://github.com/apache/flink/pull/22359#issuecomment-1538423656
Good god what is github doing. First it breaks on the merge, now it arbitrarily closes the PR.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1159528643
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,13 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>${jsonpath.version}</version>
+ <scope>test</scope>
Review Comment:
This seems incorrect. If it has a test scope then the loader won't bundle it, nor will it ever make it's way to the kafka tests (since test dependencies aren't transitive).
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1160394865
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,13 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>${jsonpath.version}</version>
+ <scope>test</scope>
Review Comment:
actually wait a second, i was only rebuilding the flink-table-planner module, i will try also rebuilding the other modules
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1162456511
##########
flink-table/flink-table-planner-loader/pom.xml:
##########
@@ -44,13 +44,11 @@
<artifactId>flink-table-api-bridge-base</artifactId>
<version>${project.version}</version>
</dependency>
-
<dependency>
<!-- Test utils -->
<groupId>org.apache.flink</groupId>
<artifactId>flink-test-utils-junit</artifactId>
</dependency>
-
Review Comment:
please revert these formatting changes
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #22359:
URL: https://github.com/apache/flink/pull/22359#issuecomment-1498148518
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c96df5042093ce7ee8ab284888d627a554c5746e",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "c96df5042093ce7ee8ab284888d627a554c5746e",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c96df5042093ce7ee8ab284888d627a554c5746e UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1160388987
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -301,6 +301,13 @@ under the License.
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ <version>${jsonpath.version}</version>
+ <scope>test</scope>
Review Comment:
Okay gotcha, i've removed the test scope. However, i am no longer able to get the `KafkaChangelogTableITCase` test running in the IDE anymore, even after rebuilding the modules on the command line. I have tried adding it individually and in various combinations in `flink-table-planner-loader-bundle, flink-table-planner-loader, flink-table-planner_${scala.binary.version}`. Did you have to do anything else to get it to work?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] nateab commented on a diff in pull request #22359: [FLINK-31660][table] add jayway json path dependency to table-planner
Posted by "nateab (via GitHub)" <gi...@apache.org>.
nateab commented on code in PR #22359:
URL: https://github.com/apache/flink/pull/22359#discussion_r1184382143
##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -392,6 +397,8 @@ under the License.
<!-- flink-table-planner dependencies -->
<include>org.apache.flink:flink-sql-parser</include>
<include>org.apache.flink:flink-sql-parser-hive</include>
+
+ <include>com.jayway.jsonpath:json-path</include>
Review Comment:
done, moved things around
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org