-
Notifications
You must be signed in to change notification settings - Fork 997
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
Upgrade Feast dependencies #854
Upgrade Feast dependencies #854
Conversation
[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.
Needs 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. Instructions 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Wirick for the cleaned up PR, and sorry for the lecture on #851 about Spring bill of materials, from hasty reading on a small screen I had thought that and #793 were from the same contributor. We can take that matter up separately I think.
Does Spring Boot 2.3.1 bring any headache? Appears to be the latest stable. There are a few test errors too, looks like trying to use H2 but Postgres driver is activating. Otherwise the upgrade is good by me overall.
I ran into problems with zip files not being supported because bigtable hbase is a large module.
Ah yes, I've been there… spring-projects/spring-boot#16091. Upgrading Spring Boot is one way to fix it, and I think Feast is due for the upgrade anyway if it didn't cause too much hell where graphs intersect with Beam, gRPC, etc. ecosystems…
It brings up a related matter we've touched on on another channel—packaging Core becomes rather bloated as more storage engine support is added to Ingestion, since Core depends on Ingestion—but that cuts so deep that it needs a new issue, extending from #529 and its friends.
@ches Thanks for the review. The failing tests are something that I tried to debug with no luck so far. the test url jdbc:h2:mem:testdb doesn't connect correctly in the integration tests but my connection to a real database in my integration tests works fine, but I haven't figured out what I'm doing wrong (So far tried using the h2 driver, switching the url property to jdbc-url and the postgres driver, etc, but no luck) Would love if anyone knew what to do |
I will 2.3.1 and see if I run into any issues |
/ok-to-test |
@Wirick This version bump requires more discussion because of the introduction of our Flyway migration scripts. I have managed to get the driver issues resolved on this branch locally, but then I start to encounter migration problems due to the incompatibility of PostreSQL and H2 database standards. The following PostreSQL commands are not functioning in H2
So we have three options
From what I have seen so far it might be a bit tricky to support H2 here, and I am wary of trying to accommodate conditional or layered migrations. The overhead would be quite high. My preference is to run these tests only with Postgres as integration tests. I believe this is something @pyalex wanted to work towards in any case. I'm not sure how that sits with teams that are not using Postgres though. @ches I believe you are still using MS SQL? |
22ac551
to
f96ebbd
Compare
Ok so I have decided to include more version bumps in this PR. It's very hard to bump versions of only one package since they are all inter-related, not to mention that these versions touch the new auth modules and our tests. I will edit the opening description. |
0a135bf
to
52c9419
Compare
/test test-end-to-end-redis-cluster |
Ok so I have tried to clear as many issues as possible. Currently I am stuck with the spec stream tests not passing. At first I thought this was related to a dependency issue in Kafka/Zookeeper, but the exceptions are the same even if I run a local Kafka or Zookeeper. It seems like the Beam pipeline never completes and just gets an interrupted exception, possibly related to acks not being received. |
Yep, I started migration of big chunk of tests (in core) to integration tests with real postgres backend (via testcontainers). And that was motivated by significant amount of bugs in current release not catched by our current tests: 1. unit tests are based on our expectations on how the whole model layer would behave and those expectation are not met too often. 2. e2e tests are mostly covering only success cases. My feeling is that though h2 may partly fix issue with tests it still won't solve it completely, because in production it still will be different database. And supporting database specific migration seems like excessive effort (though I was not aware that someone use in prod something other than Postgres when flyway was added). And is there any benefits to use h2 over postgres in tests except for maybe faster setup (which apparently run once)? |
Sorry wasn't at a keyboard for the weekend, I will take a look at this updated pr and see if I can make progress |
@woop When I execute make build-java I get this warning
I have encountered previously and was assuming that's why there was a problem with acking specs. I'm not sure how to make this error go away and where to add the repo, I feel like I've tried a couple different poms |
/test test-end-to-end-auth |
/test test-end-to-end-redis-cluster |
@Wirick: The following tests failed, say
Instructions 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. I understand the commands that are listed here. |
Resolved by #876 |
Context: I'm working on an integration with bigtable as an online store. I ran into problems with zip files not being supported because bigtable hbase is a large module. When I update spring boot there were a number of other cascading dependency issues that needed to be resolved. Currently this will allow for the work in #833 to be discussed separately from the dependency updates
This induces upgrades for a number of different packages in order to be
compatible, including kafka and the jakarta validation-api
Resolved logging errors in core/serving from seeing different logger
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.
UPDATE: I can also run my bigtable serving integration with these 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
My original PR #851 had formatting changes so I am closing in favor of this pr.
Edit from @woop:
I have included more version bumps of related dependencies because it's too difficult to incrementally upgrade them one at a time. The following packages have been upgraded.