Skip to content

Conversation

@mnoszczak
Copy link
Contributor

Prepares export_v2 methods to use streamable backend under the hood

@mnoszczak mnoszczak requested a review from a team as a code owner March 28, 2024 15:19
@mnoszczak mnoszczak requested a review from attila-papai March 28, 2024 15:20
filters: Union[CatalogExportFilters, Dict[str, List[str]], None] = None,
params: Optional[CatalogExportParams] = None,
) -> Task:
) -> Union[Task, ExportTask]:
Copy link
Contributor

Choose a reason for hiding this comment

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

In this PR we return a union or a tuple in several places, is it possible to avoid that ? Here for instance I can't tell the difference between Task and ExportTask. Also we end up not using one of the two return values in other places.

Copy link
Contributor Author

@mnoszczak mnoszczak Apr 2, 2024

Choose a reason for hiding this comment

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

This PR modifies ExportTask so that it is a superset of Task and behaves similarly. We cannot know what is being returned until we toggle the flag for all customers. We'll be able to narrow down the type in future version (once flag is enabled for everyone).

@mnoszczak mnoszczak force-pushed the mno/exports-v2-streamable branch from f942843 to 6096995 Compare April 3, 2024 10:01
@mnoszczak mnoszczak force-pushed the mno/exports-v2-streamable branch from aeeb8d0 to 02aa1ed Compare April 3, 2024 10:09

def __init__(self, task: Task) -> None:
def __init__(self, task: Task, is_export_v2: bool = False) -> None:
self._is_export_v2 = is_export_v2
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we need to explicitly pass it in because of the api feature flag that might change streamable to export_v2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value comes down from API though. The flow is as follows:

  1. New SDK version: Hey API I'm able to handle streamable backend
  2. API: Ok let me check feature flag if I can serve you streamable export
  3. SDK Receives streamable response: uses ExportTask internal (with is_export_v2=True) and tweaks its public API so it behaves just like Task class (result_url/errors_url)

@mnoszczak mnoszczak merged commit d59a616 into develop Apr 3, 2024
@mnoszczak mnoszczak deleted the mno/exports-v2-streamable branch April 3, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants