Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

feat(data-catalog): Adding Thriftserver #228

Merged

Conversation

tumido
Copy link
Contributor

@tumido tumido commented Nov 3, 2020

Adding Thriftserver component Data catalog.

Part of: https://github.com/opendatahub-io/odh-manifests/issues/222, https://github.com/opendatahub-io/odh-manifests/issues/105, DATAHUB-2294

Based on reference implementation in AICoE#29 for Internal DH.

It will need further cleanup and extraction of parts into overlays later on (storage class, externalize the database). This is expected to happen in consecutive PRs.

@openshift-ci-robot
Copy link
Collaborator

Hi @tumido. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@tumido tumido changed the title [wip] feat(data-catalog): Adding Thriftserver feat(data-catalog): Adding Thriftserver Nov 3, 2020
@tumido
Copy link
Contributor Author

tumido commented Nov 4, 2020

cc @maulikjs, @rimolive, @accorvin

@tumido
Copy link
Contributor Author

tumido commented Nov 5, 2020

/retest

failure unrelated to PR

@tumido tumido requested a review from rimolive November 5, 2020 18:01
@tumido
Copy link
Contributor Author

tumido commented Nov 10, 2020

/retest

test failures not related to the code change

@anishasthana
Copy link
Member

/retest

@tumido
Copy link
Contributor Author

tumido commented Nov 10, 2020

/retest

thriftserver/thriftserver/base/thriftserver-secret.yaml Outdated Show resolved Hide resolved
thriftserver/thriftserver/base/thriftserver-secret.yaml Outdated Show resolved Hide resolved
thriftserver/thriftserver/base/params.env Outdated Show resolved Hide resolved
@tumido tumido force-pushed the datacatalog-thriftserver branch 2 times, most recently from e8effd9 to c76777e Compare November 12, 2020 10:12
@tumido
Copy link
Contributor Author

tumido commented Dec 8, 2020

Rebased to master. Tests are flaky again. I'll retest it later.

@tumido
Copy link
Contributor Author

tumido commented Dec 10, 2020

/retest

@tumido tumido force-pushed the datacatalog-thriftserver branch 2 times, most recently from 94c93fc to 7d94878 Compare December 10, 2020 15:26
- create-spark-cluster
parameters:
- name: s3_endpoint_url
value: s3.odh.com
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the protocol required here if we need to use unsecured http?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so you need to do that. As you can see here, we use it like this in operate first and it works just fine:

https://github.com/operate-first/apps/blob/d2b355fb2f94cff08dc0e2053711e105e635970e/odh/base/datacatalog/kfdef.yaml#L43-L54

@tumido tumido requested a review from LaVLaS December 11, 2020 09:38
@crobby
Copy link
Contributor

crobby commented Dec 14, 2020

@crobby
Copy link
Contributor

crobby commented Dec 14, 2020

The tests for this actually failed: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/opendatahub-io_odh-manifests/228/pull-ci-opendatahub-io-odh-manifests-master-odh-manifests-e2e/1337056179376885760/artifacts/odh-manifests-e2e/e2e/container-logs/test.log

Not sure why it was reported as a success. I'll take a look at the test wrapper.

Ok, I have spotted the issue with the tests and have a PR up to fix that problem: #267
Once that merges, a rebase will be necessary (and the tests will likely report failure as it should).

Copy link
Member

@LaVLaS LaVLaS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

I verified this with hue PR #227 and the opendatahub.io data exploration tutorial

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LaVLaS, tumido

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@tumido
Copy link
Contributor Author

tumido commented Dec 16, 2020

Rebased 👍

@maulikjs
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link
Collaborator

@maulikjs: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@LaVLaS
Copy link
Member

LaVLaS commented Jan 5, 2021

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit ca177f4 into opendatahub-io:master Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet