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

Remove unused Hibernate dep from Serving #721

Merged
merged 1 commit into from
May 22, 2020

Conversation

ches
Copy link
Member

@ches ches commented May 21, 2020

What this PR does / why we need it:

Removes an unused dependency. Just a little housekeeping noticed in the context of something related. Core and Serving were using different versions of Hibernate, which I first wanted to align, then thought to myself, "wait, how does Serving use Hibernate?"

Maybe someone knows the background of what "Hibernate for formatting SQL string" meant, but grepping/mvn dependency:analyze/building suggests it is indeed unused.

Which issue(s) this PR fixes:

No issue

Does this PR introduce a user-facing change?:

NONE

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ches
To complete the pull request process, please assign davidheryanto
You can assign the PR to them by writing /assign @davidheryanto in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ches
Copy link
Member Author

ches commented May 21, 2020

/assign @davidheryanto

@woop
Copy link
Member

woop commented May 21, 2020

Flaky tests are killing me.

@ches
Copy link
Member Author

ches commented May 21, 2020

I tried one rerun of the Docker Compose workflow, still got timeouts. Looks like it's failing on every recent PR.

@woop
Copy link
Member

woop commented May 21, 2020

I tried one rerun of the Docker Compose workflow, still got timeouts. Looks like it's failing on every recent PR.

Ah, it's supposed to be failing. Yay. So it's not flakiness, it's due to not updating the SDK to the latest version. Will have a look a bit later.

And documentation is still pending for me.

@davidheryanto
Copy link
Collaborator

/lgtm
I think the Hibernate library was used last time to format the SQL query we generate for querying historical storage (i.e BigQuery). But yeah it seems we no longer use it.

@woop
Copy link
Member

woop commented May 21, 2020

I tried one rerun of the Docker Compose workflow, still got timeouts. Looks like it's failing on every recent PR.

I believe I have fixed the docker compose test, but for some reason your test isnt using the code that is on the master branch. It should be cloning the v0.5-branch into the docker compose tests.

I think you should maybe just rebase and try again.

Might be smarter to only run these Docker Compose tests on master and not on PRs.

@ches ches force-pushed the one-hibernate branch from 00f4d2b to 9563189 Compare May 21, 2020 17:32
@feast-ci-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@feast-ci-bot feast-ci-bot removed the lgtm label May 21, 2020
@ches
Copy link
Member Author

ches commented May 21, 2020

/retest

Docker Compose Actions workflow still appears to time out.

@woop woop merged commit ec7d413 into feast-dev:master May 22, 2020
@ches ches deleted the one-hibernate branch May 23, 2020 04:37
khorshuheng pushed a commit to khorshuheng/feast that referenced this pull request Jun 5, 2020
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.

4 participants