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

Cache Serialize API #215

Merged
merged 10 commits into from
Nov 23, 2021

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Aug 24, 2021

TL;DR

Removes the ExtendReservation API from the datacatalog protobuf definition.

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

The datacatalog ExtendReservation API is continually called when an Artifact is already reserved and that reservation needs to be extended past the current expiration. We are changing the implementation of the GetOrReserveArtifact API to extend a reservation if valid. Therefore, ExtendReservation is no longer necessary. We have included additional fields into the GetOrReserveArtifactResponse to reflect these changes.

Tracking Issue

flyteorg/flyte#267
flyteorg/flyte#872

Follow-up issue

NA

@welcome
Copy link

welcome bot commented Aug 24, 2021

Thank you for opening this pull request! 🙌
These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge. - Sign off your commits (Reference: DCO Guide).

@hamersaw hamersaw changed the title Removing datacatalog ExtendReservation API Remove datacatalog ExtendReservation API Aug 24, 2021
@hamersaw hamersaw changed the title Remove datacatalog ExtendReservation API [WIP] Remove datacatalog ExtendReservation API Aug 25, 2021
@hamersaw hamersaw marked this pull request as draft August 25, 2021 17:38
@hamersaw hamersaw marked this pull request as ready for review September 28, 2021 12:56
@hamersaw hamersaw changed the title [WIP] Remove datacatalog ExtendReservation API Cache Serialize API Sep 28, 2021
@hamersaw hamersaw force-pushed the refactor/artifact-reservation-api branch from 166791c to 100abab Compare November 3, 2021 11:59
EngHabu
EngHabu previously approved these changes Nov 13, 2021
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Finally started looking at the cache serialization PRs...
Let's wait on merging them until they are all +1'd

protos/flyteidl/core/catalog.proto Outdated Show resolved Hide resolved
EngHabu
EngHabu previously approved these changes Nov 18, 2021
@@ -92,6 +92,9 @@ message TaskMetadata {
oneof interruptible_value {
bool interruptible = 8;
};

// Indicates whether the system should attempt to execute discoverable instances in serial to avoid duplicate work
bool discovery_serializable = 9;
Copy link
Contributor

Choose a reason for hiding this comment

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

i hate this name. I guess we want to keep it so.
but, ideally it should be cache_serializable right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. i'm ok with the switch, it hurt me to try and keep it uniform.

kumare3
kumare3 previously approved these changes Nov 19, 2021
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.

one comment

@hamersaw hamersaw dismissed stale reviews from kumare3 and EngHabu via baebf8a November 23, 2021 17:08
@EngHabu EngHabu merged commit 72b7e5e into flyteorg:master Nov 23, 2021
@hamersaw hamersaw deleted the refactor/artifact-reservation-api branch December 1, 2021 18:38
eapolinario pushed a commit that referenced this pull request Sep 8, 2023
* removed datacatalog ExtendReservation API

Signed-off-by: Daniel Rammer <[email protected]>

* filled out generated content with 'make generate'

Signed-off-by: Daniel Rammer <[email protected]>

* updated protobuf docs for admin and core definitions

Signed-off-by: Daniel Rammer <[email protected]>

* created ReservationID message to reuse

Signed-off-by: Daniel Rammer <[email protected]>

* changed artifact reservation API to not include handling of artifacts - only reservations.

Signed-off-by: Daniel Rammer <[email protected]>

* added cache reservation comments

Signed-off-by: Daniel Rammer <[email protected]>

* fixed indenting issues that always drive me crazy

Signed-off-by: Daniel Rammer <[email protected]>

* updated discovery_reservable to discovery_serializable to adhere to name change

Signed-off-by: Daniel Rammer <[email protected]>

* moved catalog reservation status enum into a message

Signed-off-by: Daniel Rammer <[email protected]>

* changed discovery_serializable TaskMetadata field to cache_serializable

Signed-off-by: Daniel Rammer <[email protected]>
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