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

Add output prefix to LaunchPlanSpec #74

Merged
merged 10 commits into from Jul 30, 2020
Merged

Add output prefix to LaunchPlanSpec #74

merged 10 commits into from Jul 30, 2020

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Jul 29, 2020

TL;DR

Please replace this text with a description of what this PR accomplishes.

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

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#211

Follow-up issue

NA

core.QualityOfService quality_of_service = 16;

// Prefix for where offloaded data from user workflows will be written (ie Blobs, Schema, query data, etc.)
string output_data_prefix = 17;
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 keep the naming consistent? call it raw_outputdata_prefix
In the docstring it can be, of the format
s3://bucket/key OR s3://bucket/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure but what's raw about it?


// Prefix for where offloaded data from user workflows will be written (i.e. Blobs, Schema, query data, etc.)
// e.g. s3://bucket/key or s3://bucket/
string raw_output_data_prefix = 17;
Copy link
Contributor

Choose a reason for hiding this comment

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

on thinking more, maybe this should be a structure. As we may want to add the policy like
Sharding desired: True
etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what else would go in?

Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

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

Lgtm

@wild-endeavor wild-endeavor merged commit c3baba8 into master Jul 30, 2020
eapolinario pushed a commit that referenced this pull request Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants