Skip to content

Remove unused Hibernate dep from Serving#721

Merged
woop merged 1 commit intofeast-dev:masterfrom
agoda-com:one-hibernate
May 22, 2020
Merged

Remove unused Hibernate dep from Serving#721
woop merged 1 commit intofeast-dev:masterfrom
agoda-com:one-hibernate

Conversation

@ches
Copy link
Copy Markdown
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
Copy Markdown
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.

Details 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
Copy Markdown
Member Author

ches commented May 21, 2020

/assign @davidheryanto

@woop
Copy link
Copy Markdown
Member

woop commented May 21, 2020

Flaky tests are killing me.

@ches
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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