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

Added Agency API #104

Merged
merged 4 commits into from
Apr 17, 2018
Merged

Added Agency API #104

merged 4 commits into from
Apr 17, 2018

Conversation

ewoutp
Copy link
Contributor

@ewoutp ewoutp commented Apr 13, 2018

This PR adds an Agency API.

Note that this API is not intended for normal database use.

@ewoutp ewoutp requested a review from neunhoef April 13, 2018 14:07
Copy link
Member

@neunhoef neunhoef left a comment

Choose a reason for hiding this comment

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

We should at least increase the maximal timeout. I do not know how far we want to try to resolve the non-idempotency problem.

// agencyConnectionFailureBackoff returns a backoff delay for cases where all
// agents responded with a non-fatal error.
func agencyConnectionFailureBackoff(lastDelay time.Duration) time.Duration {
return increaseDelay(lastDelay, 1.5, time.Millisecond, time.Second*2)
Copy link
Member

Choose a reason for hiding this comment

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

I fear that a maximum delay of 2 seconds will not be enough in cases in which the machine is under a heavy load. I suggest to increase the maximum considerably to maybe 15 or 30 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why. The result of this function is used as a delay period between failing requests. It is not the timeout for an agency request.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then I misunderstood this, sorry. Please ignore.

cancel()
close(results)
close(errors)
if result, ok := <-results; ok {
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand what happens if a 307 is returned. Is this reported as an error? Or does each request follow the redirect? Both would be bad. I think if we send to all agents we need to ignore 307, but I do not understand how this works here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underling HTTP connection is configured to fail in redirect. That means a 302 error is returned. The code here will consider any non-permanent failure (everything in the 300 range) as "this agent did not handle, so we'll wait for another".

Only in case all agents did not respond with either a result or a permanent failure, we'll retry the operation.
So the notion of automatic retry does not exist in any other case.

Copy link
Member

Choose a reason for hiding this comment

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

Good, understood, thanks.

// WriteKey writes the given value with the given key with a given TTL (unless TTL is zero).
// If you pass a condition (only 1 allowed), this condition has to be true,
// otherwise the write will fail with a ConditionFailed error.
func (c *agency) WriteKey(ctx context.Context, key []string, value interface{}, ttl time.Duration, condition ...WriteCondition) error {
Copy link
Member

Choose a reason for hiding this comment

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

In this function an automatic retry is OK, since it is idempotent.

}

// WriteKeyIfEmpty writes the given value with the given key only if the key was empty before.
func (c *agency) WriteKeyIfEmpty(ctx context.Context, key []string, value interface{}, ttl time.Duration) error {
Copy link
Member

Choose a reason for hiding this comment

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

Here, a retry can be disastrous, since the operation is not idempotent. This is actually a very difficult problem. There is the possibility to send a client ID along with a transaction, such that one is later able to enquire if an operation has actually happened already. However, this makes the client code quite difficult to get right (see our AgencyComm, we had considerable trouble to implement this correctly, LOL, and I am not sure we got it right now). Furthermore, even that is not entirely water-tight because of the rather fundamental two generals problem. We never know whether the first request is sent of and lingers in some TCP buffer, and is sent after a longish delay. During the time when it is in the buffers, it is not possible to either stop it or to find out if it will be executed eventually. I do not know what the right approach is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above


// WriteKeyIfEqualTo writes the given new value with the given key only if the existing value for that key equals
// to the given old value.
func (c *agency) WriteKeyIfEqualTo(ctx context.Context, key []string, newValue, oldValue interface{}, ttl time.Duration) error {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.


// RemoveKeyIfEqualTo removes the given key only if the existing value for that key equals
// to the given old value.
func (c *agency) RemoveKeyIfEqualTo(ctx context.Context, key []string, oldValue interface{}) error {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@ewoutp ewoutp merged commit ce82333 into master Apr 17, 2018
@ewoutp ewoutp deleted the agency branch April 17, 2018 08:20
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.

2 participants