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

Decoupling DB interface from katib manager #498

Closed
wants to merge 59 commits into from

Conversation

achalshant
Copy link

@achalshant achalshant commented May 11, 2019

Goal: To decouple the DB interface from katib manager in order to make the backend store pluggable. Please refer to the proposal doc for more details - https://docs.google.com/document/d/1xAi4V9SCAVZvrwiVc6a3VpK7BXPx-liQbr6YPdqBThs/edit#heading=h.1hri22e2jta

Major changes:

pkg/db/interface.go was moved to run as a service in cmd/dbif/mysql/main.go
cmd/manager/main.go is now a thin client that connects to the DBIF service using GRPC and operates on the DB.
Some services and messages from api.proto have moved to dbif.proto.


This change is Reviewable

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

pkg/api/v1alpha2/build.sh Outdated Show resolved Hide resolved
cmd/manager/v1alpha2/main.go Outdated Show resolved Hide resolved
@YujiOshima
Copy link
Contributor

please move pkg/db/v1alpha2/db_init.go and pkg/db/v1alpha2/interface_test.go to cmd/dbif/mysql/ since they are only for mysql.

@achalshant achalshant reopened this May 21, 2019
@achalshant
Copy link
Author

@hougangliu @YujiOshima

@hougangliu
Copy link
Member

@achalshant thanks for your PR. I am afraid that we cannot include this feature in 0.6 release.
@johnugeorge @richardsliu @gaocegege any comment?

@richardsliu
Copy link
Contributor

Agree, we can try to take it up in the next release in June or July.

@hougangliu
Copy link
Member

@achalshant will you pick up this PR now?

@achalshant
Copy link
Author

Okay @hougangliu .

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign johnugeorge
You can assign the PR to them by writing /assign @johnugeorge 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

@k8s-ci-robot
Copy link

@achalshant: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
kubeflow-katib-presubmit aa9eb3f link /test kubeflow-katib-presubmit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@hougangliu hougangliu mentioned this pull request Aug 26, 2019
5 tasks
@johnugeorge
Copy link
Member

DB interface is introduced in #685

@johnugeorge
Copy link
Member

Closing this issue as the backend is pluggable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants