Conversation
1. This induces upgrades for a number of different packages in order to be compatible, including kafka and the jakarta validation-api 2. Resolved logging errors in core/serving from seeing different logger classes in the classpath by excluding them
Wirick
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Wirick The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @Wirick. Thanks for your PR. I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
The re-indentation is a bit distracting, I know some POM files in the project are indented with two spaces and others four, and GitHub has the option to ignore whitespace in diffs, but if we're going to do mass reformats it'd be nice to do them in separate commits and ideally with a means to check/automate the convention going forward. Something that could have been brought up in #793 I think: we use an imported Spring bill of materials from the root Spring and Beam or proto/gRPC eco dep versions may be at odds sometimes so may necessitate overrides, but at least as long as Core depends on Ingestion I believe it's worth maintaining the BOM usage. If/when that situation changes it might make sense to move service API modules under their own parent POM holding the Spring BOM. |
| <goal>jar</goal> | ||
| </goals> | ||
| <configuration> | ||
| <javadocExecutable>${java.home}/bin/javadoc</javadocExecutable> |
There was a problem hiding this comment.
Any reason this needed to be set specifically?
There was a problem hiding this comment.
Yea my intellij environment ran into the issue described and solved here https://intellij-support.jetbrains.com/hc/en-us/community/posts/206880855-MavenReportException-Error-while-creating-archive-Unable-to-find-javadoc-command-The-environment-variable-JAVA-HOME-is-not-correctly-set- so thats what allowed me to execute this in intellij. We can continue the convo here #854
ches
left a comment
ches
left a comment
There was a problem hiding this comment.
Okay looking at it without "Hide whitespace changes" the re-indentation is more than "a bit" distracting—let's please revert unnecessary whitespace changes. It's a pain for merge conflicts, git blame, etc. If we want more consistent XML formatting, let's propose that separately.
|
Closing in favor of #854 |
3 participants
compatible, including kafka and the jakarta validation-api
classes in the classpath by excluding them
I am able to build and run feast core and feast serving, along with bigquery batch ingestion with these updated dependencies. However there was an upgrade needed in how we mock the url for the test database. Context: https://stackoverflow.com/questions/49088847/after-spring-boot-2-0-migration-jdbcurl-is-required-with-driverclassname