Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds support for references in the descriptor #949

Merged
merged 9 commits into from Oct 4, 2019

Conversation

@shamsimam
Copy link
Contributor

commented Sep 10, 2019

Changes proposed in this PR

  • adds support for references in the descriptor
  • references are now used to determine if a service is stale

Why are we making these changes?

We need to allow custom builders to add references to services and also contribute to marking services as stale rather than relying only on tokens going stale.

@shamsimam shamsimam added the wip label Sep 10, 2019
@shamsimam shamsimam self-assigned this Sep 10, 2019
@shamsimam shamsimam force-pushed the descriptor-references branch 6 times, most recently from e44b268 to 948a03e Sep 10, 2019
@shamsimam shamsimam added internal-green and removed wip labels Sep 12, 2019
@shamsimam shamsimam requested a review from sradack Sep 12, 2019
references (cond-> references
(seq source-tokens) (conj {:sources source-tokens :type :token})
;; if no references were used to create the service, it is directly accessible
(and (empty? references) (empty? source-tokens)) (conj {:type :direct-access}))]

This comment has been minimized.

Copy link
@sradack

sradack Sep 16, 2019

Contributor

How about just :direct?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Sep 16, 2019

Author Contributor

Changed.

Copy link
Contributor Author

left a comment

Addressed feedback.

references (cond-> references
(seq source-tokens) (conj {:sources source-tokens :type :token})
;; if no references were used to create the service, it is directly accessible
(and (empty? references) (empty? source-tokens)) (conj {:type :direct-access}))]

This comment has been minimized.

Copy link
@shamsimam

shamsimam Sep 16, 2019

Author Contributor

Changed.

@shamsimam shamsimam added wip and removed internal-green labels Sep 16, 2019
@shamsimam shamsimam force-pushed the descriptor-references branch from 6c0eb6d to 276e138 Sep 16, 2019
@shamsimam shamsimam added internal-green and removed wip labels Sep 17, 2019
@shamsimam

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@sradack ready for another round of review

@shamsimam shamsimam force-pushed the descriptor-references branch from 276e138 to bbf6251 Sep 23, 2019
@shamsimam shamsimam added wip and removed internal-green labels Sep 23, 2019
@shamsimam

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

Rebasing changes to make it easier to merge.

@shamsimam shamsimam force-pushed the descriptor-references branch from bbf6251 to 2c92b30 Sep 26, 2019
@shamsimam shamsimam added internal-green and removed wip labels Sep 26, 2019
@shamsimam shamsimam force-pushed the descriptor-references branch from 2c92b30 to 37157a0 Sep 27, 2019
@shamsimam

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

@sradack this is ready for another round of review.

(let [lock-path (str base-path "/" path)]
(curator/synchronize curator lock-path mutex-timeout-ms f))))})
(let [lock-path (str base-path "/" path)
lock-name (apply str (filter #(Character/isLetterOrDigit ^char %) (str path)))]

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

Please use a regex replace.

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

synchronize-fn is accepting a path. The caller should be responsible for valid paths.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Done, caller provides correct path.

## Service GC

By default, Waiter relies on the service idle timeout to GC services after periods of inactivity (not receiving requests).
However, service reachable only via references, e.g. tokens, can be GC-ed eagerly if the reference has been updated.

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

service->services

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Done.

However, service reachable only via references, e.g. tokens, can be GC-ed eagerly if the reference has been updated.

When a service description is constructed from a request, the service references are also updated.
These references are available as the `:reference` key in the descriptor.

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

Why not :references?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Changed to :reference-type->entry


When a service description is constructed from a request, the service references are also updated.
These references are available as the `:reference` key in the descriptor.
The :refrence enrty is a map where the keys represent a `:type` parameter.

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

:refrence -> :reference

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

enrty -> entry

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

It would be great to show an example here.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Fixed typos, added example.

`retrieve-reference-type->stale-fn` of the builder and invoking the corresponding 'type' function on the value.

The default implementation of the `ServiceDescriptionBuilder` returns
the following functions for the different reference types:

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

It returns two functions?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Changed description to clarify it returns a map.

(when (-> (service-id->references kv-store service-id)
(or #{})
(contains? reference)
(not))

This comment has been minimized.

Copy link
@sradack

sradack Oct 1, 2019

Contributor

Can change this to a when-not?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Changed.

Copy link
Contributor Author

left a comment

Addressed feedback

## Service GC

By default, Waiter relies on the service idle timeout to GC services after periods of inactivity (not receiving requests).
However, service reachable only via references, e.g. tokens, can be GC-ed eagerly if the reference has been updated.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Done.

However, service reachable only via references, e.g. tokens, can be GC-ed eagerly if the reference has been updated.

When a service description is constructed from a request, the service references are also updated.
These references are available as the `:reference` key in the descriptor.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Changed to :reference-type->entry


When a service description is constructed from a request, the service references are also updated.
These references are available as the `:reference` key in the descriptor.
The :refrence enrty is a map where the keys represent a `:type` parameter.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Fixed typos, added example.

`retrieve-reference-type->stale-fn` of the builder and invoking the corresponding 'type' function on the value.

The default implementation of the `ServiceDescriptionBuilder` returns
the following functions for the different reference types:

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Changed description to clarify it returns a map.

(let [lock-path (str base-path "/" path)]
(curator/synchronize curator lock-path mutex-timeout-ms f))))})
(let [lock-path (str base-path "/" path)
lock-name (apply str (filter #(Character/isLetterOrDigit ^char %) (str path)))]

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Done, caller provides correct path.

(when (-> (service-id->references kv-store service-id)
(or #{})
(contains? reference)
(not))

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Changed.

The `reference-type->entry` is a map where the keys represent a `:type` parameter, e.g.
a service created with `x-waiter-token: foo` header on a request will have the value:
```
{:token {:sources [{"token" "foo" "version" "v1"}]}}

This comment has been minimized.

Copy link
@sradack

sradack Oct 2, 2019

Contributor

Why are we using strings for the "token" and "version" keys?

This comment has been minimized.

Copy link
@sradack

sradack Oct 2, 2019

Contributor

How are multiple tokens handled? Is that modeled differently from various tokens resolving to the same service?

This comment has been minimized.

Copy link
@sradack

sradack Oct 2, 2019

Contributor

How does this map change if the same service was to be resolved directly without a token?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Changed code to store keys.

The map is per requests. Multiple reference-type->entry end up being stored in a set for a service when a service is accessed via multiple paths. Added example to clarify.

`reference-type->entry` maps as references that refer to the service.
The service GC process checks individual references by type and marks a service as a candidate for eager GC
if _all_ references used to access the service are stale.
An individual reference, i.e. the `reference-type->entry` map, is stale if any of its value entries is stale.

This comment has been minimized.

Copy link
@sradack

sradack Oct 2, 2019

Contributor

I don't follow this sentence. The reference-type->entry map may contain multiple references, right?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Rephrased, let me know if it helps.

This staleness check on the value is performed using the functions returned from
`retrieve-reference-type->stale-fn` of the builder and invoking the corresponding 'type' function on the value.

The default implementation of the `ServiceDescriptionBuilder` returns a map with a single entry for `:token`.

This comment has been minimized.

Copy link
@sradack

sradack Oct 2, 2019

Contributor

from the retrieve-reference-type->stale-fn?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Added.

E.g. a hypothetical implementation which treats the image parameter as docker images can
have the following entry for a service:
```
{:image {:name "twosigma/kitchen" :build "20191001"}

This comment has been minimized.

Copy link
@sradack

sradack Oct 2, 2019

Contributor

Couldn't we have multiple image labels resolving to the same service?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 2, 2019

Author Contributor

Yes, they would show up as different entries in the references. The retrieve-reference-type->stale-fn is computed per request.

This comment has been minimized.

Copy link
@sradack

sradack Oct 3, 2019

Contributor

Let's use the opportunity of my confusion to improve the docs and make this more explicit.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Oct 3, 2019

Author Contributor

Documentation has been updated.

This comment has been minimized.

Copy link
@sradack

sradack Oct 4, 2019

Contributor

This is much better now. Thank you for the updates.

waiter/docs/service-references.md Show resolved Hide resolved
@shamsimam

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@sradack ready for another round of review.

@sradack
sradack approved these changes Oct 4, 2019
E.g. a hypothetical implementation which treats the image parameter as docker images can
have the following entry for a service:
```
{:image {:name "twosigma/kitchen" :build "20191001"}

This comment has been minimized.

Copy link
@sradack

sradack Oct 4, 2019

Contributor

This is much better now. Thank you for the updates.

:service-id->password-fn (pc/fnk [[:scheduler service-id->password-fn*]]
service-id->password-fn*)
:service-id->references-fn (pc/fnk [[:state kv-store]]
(partial sd/service-id->references kv-store))

This comment has been minimized.

Copy link
@sradack

sradack Oct 4, 2019

Contributor

Can we avoid partial? It's quite slow and this is in the critical path.

This comment has been minimized.

Copy link
@sradack

sradack Oct 4, 2019

Contributor

I was curious about this and dug a bit further.

https://stuartsierra.com/2015/06/01/clojure-donts-optional-arguments-with-varargs

Were you aware that calling varargs functions always allocates a new sequence?

https://gist.github.com/halgari/5d7780e6f068ae217bb2

We use these a lot and I wonder if we can improve performance and reduce GC pressure by avoiding these.

This comment has been minimized.

Copy link
@sradack

sradack Oct 4, 2019

Contributor

Separate PR though

@sradack sradack merged commit 56213be into master Oct 4, 2019
2 checks passed
2 checks passed
Mergeable Mergeable Run has been Completed!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sradack sradack deleted the descriptor-references branch Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.