Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Doc hub proposal #452

Merged
merged 84 commits into from
Dec 28, 2022
Merged

Doc hub proposal #452

merged 84 commits into from
Dec 28, 2022

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Jun 30, 2022

TL;DR

Design Doc

Database Schema

Task Table

Name Type
... ...
descriptionID uint32

Description Table

Name Type
resource string
project string
domain string
name string
version string
descriptionID uint32
Digest []byte
ShortDescription string
LongDescription []byte
link string

Create a task/workflow description

we have added description_entity to taskSpec / workflowSpec, so we'll register a description entity when we register a task / workflow.

  1. Check if we have the same description entity.
Select * from Description WHERE resource = task AND project = flytesnacks AND domain = development AND NAME = tf_wf AND version = xyz
  1. If the same description entity doesn't exist, then insert a new row to the Description Table.
  2. update descriptionID in the specific row in the task table.

Get task description

Use descriptionID in the task table to find the specific description entity

Select description.* LEFT JOIN task ON description.descriptionID = task.descriptionID AND description.project = task.project AND description.domain = task.domain AND description.name = task.name WHERE task.project = flytesnacks AND task.domain = development AND NAME = task.tf_wf AND task.version = a123456

List task description

Select description.* LEFT JOIN task ON description.descriptionID = task.descriptionID AND description.project = task.project AND description.domain = task.domain AND description.name = task.name WHERE task.project = flytesnacks AND task.domain = development AND NAME = task.tf_wf LIMIT 20

Get / List the Task

Added short description to task proto, so we'll get short description when we get / list the task.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

^^^

Tracking Issue

flyteorg/flyte#531

Follow-up issue

NA

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #452 (8e1f17d) into master (ee8de0f) will increase coverage by 0.09%.
The diff coverage is 61.57%.

@@            Coverage Diff             @@
##           master     #452      +/-   ##
==========================================
+ Coverage   59.98%   60.07%   +0.09%     
==========================================
  Files         164      168       +4     
  Lines       14628    14997     +369     
==========================================
+ Hits         8774     9010     +236     
- Misses       5086     5196     +110     
- Partials      768      791      +23     
Flag Coverage Δ
unittests 60.07% <61.57%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/repositories/config/migrations.go 2.98% <0.00%> (-0.22%) ⬇️
pkg/repositories/gorm_repo.go 0.00% <0.00%> (ø)
pkg/repositories/gormimpl/common.go 62.85% <ø> (ø)
pkg/rpc/adminservice/base.go 3.53% <0.00%> (-0.07%) ⬇️
pkg/rpc/adminservice/description_entity.go 0.00% <0.00%> (ø)
pkg/repositories/gormimpl/task_repo.go 68.26% <33.33%> (-5.93%) ⬇️
pkg/manager/impl/task_manager.go 64.00% <50.00%> (-0.93%) ⬇️
pkg/repositories/gormimpl/workflow_repo.go 68.04% <53.84%> (-0.85%) ⬇️
pkg/manager/impl/workflow_manager.go 56.72% <54.54%> (-0.26%) ⬇️
...g/repositories/gormimpl/description_entity_repo.go 65.82% <65.82%> (ø)
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Copy link
Member Author

pingsutw commented Nov 2, 2022

cc @katrogan mind taking a look

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@@ -15,5 +15,10 @@ type Workflow struct {
TypedInterface []byte
RemoteClosureIdentifier string `gorm:"not null" valid:"length(0|255)"`
// Hash of the compiled workflow closure
Digest []byte
Digest []byte
DescriptionID uint `gorm:"index"`
Copy link
Contributor

Choose a reason for hiding this comment

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

so this will always create a record in the description_entities table, even if the task/wf has no comment. do we want this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use gorm conventions to make this relational? https://gorm.io/docs/has_one.html

Version: id.Version,
},
ShortDescription: descriptionEntity.ShortDescription,
LongDescription: longDescriptionBytes,
Copy link
Contributor

Choose a reason for hiding this comment

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

in an earlier version of this code, the long description was in S3 or a blob store of some kind right? it now will store everything in the db?

Copy link
Contributor

Choose a reason for hiding this comment

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

What are the length restrictions on the long description?

Copy link
Member Author

Choose a reason for hiding this comment

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

4KB, flytekit will offload the long description if it exceeds 4KB when registering

@wild-endeavor
Copy link
Contributor

and should that left join be a right join katrina?

// ShortDescription is saved in the description entity table. set this to read only so we won't create this column.
// Adding ShortDescription because we want to unmarshal the short description in the
// descriptionEntity table to workflow object.
ShortDescription string `gorm:"<-:false"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is stored in the descriptions table, we'll have to do a left join now on every GetTask and GetWorkflow to populate the short description? Any reason not to store it inline?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated, store it inline now

if tx.Error != nil {
return r.errorTransformer.ToFlyteAdminError(tx.Error)
}
r.db.Last(descriptionEntity)
Copy link
Contributor

@katrogan katrogan Nov 29, 2022

Choose a reason for hiding this comment

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

you can use FirstOrCreate when you create the description entity to get its db id and avoid this lookup

metrics gormMetrics
}

func (r *DescriptionEntityRepo) Create(ctx context.Context, input models.DescriptionEntity) (uint, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

who calls this since you always Create as a side effect of workflow and task registration?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, I forgot to remove this. removed it.

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Copy link
Member Author

updating the tests

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Copy link
Member Author

some updates:

  • Add more tests
  • Store short description in task and workflow table, so we don't to join description entity table to get the short description.
  • Remove descriptionID from task and workflow table because we don't need to join description entity table.

@pingsutw pingsutw merged commit d2215ed into master Dec 28, 2022
@pingsutw pingsutw deleted the doc-hub branch December 28, 2022 19:06
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Doc Hub Proposal

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix test error

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* more tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update database schema

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fixed tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fixed tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* More tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update idl

Signed-off-by: Kevin Su <pingsutw@apache.org>

* list description entity

Signed-off-by: Kevin Su <pingsutw@apache.org>

* more tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* register docs when creating task

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update go.sum

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update idl

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* more tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Add short description to workflow

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix test

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix test

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix test

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix test

Signed-off-by: Kevin Su <pingsutw@apache.org>

* more test

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update to one transation

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update comment

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Bump idl

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Add identifier

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update idl

Signed-off-by: Kevin Su <pingsutw@apache.org>

* wip

Signed-off-by: Kevin Su <pingsutw@apache.org>

* merged master

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Address comment

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Address comment

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update migrations.go

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Merged master

Signed-off-by: Kevin Su <pingsutw@apache.org>

* more tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

Signed-off-by: Kevin Su <pingsutw@apache.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants