match retry policy with go etcd client

[lavacat]

No description provided.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lavacat
Once this PR has been reviewed and has the lgtm label, please assign fanminshi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[lburgazzoli]

@lavacat thx for the PR, as you have noticed, I do have a very limited amount of time to dedicate to the project, so this PR is much appreciated

[jcferretti]

I just found this PR after #1445 had already been merged.

It would have helped if this PR had any linking information to the jepsen report and its dicussion in etcd github issues (that I assumed motivated it); as it was created, there was no context or motivation mentioned. The description in this PR is totally empty, and there is no associated etcd or jetcd issue.

As I mentioned in the discussion thread in etcd-io/etcd#18424 (comment), I don't believe the conditions

return desc.equals("there is no address available") || desc.equals("there is no connection available");

can happen (I don't think the expression in that return can be true), since I do not think UNAVAILABLE is being generated client side here, but actually being sent by the server.

[vorburger]

@lavacat @jcferretti @lburgazzoli I don't know the history of this PR, but I would like to help keeping this repo "clean".

Seeing how this PR does not build and has merge conflicts, would you like to rebase and fix it up?

Without hearing from you within ~1 month from today (specifically by Sunday August 31st), I'll take the liberty to close this old PR.

[lavacat]

@vorburger sorry about slow reply. I'll try to rebase, give me some time

[jcferretti]

I believe there is no point in this PR as #1445 is already merged and it addresses the root problem. There is also some confusion about the whole issue in terms of what was happening in jetcd per jepsen and the different ways the jetcd and go clients work (which I believe make some assumptions in the code in this PR incorrect, see my previous comment). It would make sense to try to take a few steps back and understand what has happened before here, both in terms of the understanding around the original jepsen report and what was happening in terms of jetcd behavior (the analysis of that is not correct in the early parts of the discussion in the upstream etcd repo between etcd team members and jepsen) and the changes that have been made since then here.

In term of my own understanding and analysis (and by all means somebody else should take the time to try to understand this and see if they agree), we should close this PR without merging it.

[lavacat]

Sorry, I got busy and didn't followup last year back when it was still fresh on my mind.

@jcferretti your PR looks good, but imo it will be better to follow the same naming convention as go client - isSafeRetryMutableRPC and isSafeRetryImmutableRPC for convenience of majority of contributors that are only familiar with go client.

Also, I wouldn't expose autoRetry option, since it's not exposed in go-client. Otherwise Jepsen can run with autoRetry = true and claim that etcd is broken.
The problem with Jepsen tests is that majority of etcd maintainers use go client and it requires non-trivial amount of time to setup jepsen/figure out jetcd differences.

Correct me if I'm wrong but your main concern was about

return desc == "there is no address available" || desc == "there is no connection available" 

You are correct, those messages never happen. I've copied from go client for completeness. No need to have them in jetcd

[jcferretti]

Sorry, I got busy and didn't followup last year back when it was still fresh on my mind.
No worries, and thanks for taking the time now.

@jcferretti your PR looks good, but imo it will be better to follow the same naming convention as go client - isSafeRetryMutableRPC and isSafeRetryImmutableRPC for convenience of majority of contributors that are only familiar with go client.

I don't see a problem with that, but there is an implicit assumption in your statement that I don't think is true: that people that make contributions on the etcd client go side also frequently make contributions in jetcd / etcd client java side. My company and my team has been using etcd with a java system via jetcd for many years, and we have been paying attention to this repo. This is the first time I see some attention from the main etcd repo people on jetcd, which I am going to assume was motivated by the visibility of jepsen and the long winded discussion around their report.

Also, I wouldn't expose autoRetry option, since it's not exposed in go-client.

The option is important for giving users of jetcd an escape hatch that allows them to continue to run their systems in the way they were running them before, since in jetcd it was the (wrong) default behavior. We are switching auto-retries off by default for the non safe cases (writes), which is a non-trivial change in behavior that can impact people. They will need to audit their codebases and figure out some decision logic on their application level side for when, and if, to auto retry. That will take time. Giving them an option gives them breathing space to do that.

Otherwise Jepsen can run with autoRetry = true and claim that etcd is broken.

autoRetry true is not the default. There are many options that can be set incorrectly and break etcd or jetcd, I don't see how this is different. Even further on the argument, you have fixed issues in the past in etcd go side by requiring people to set options different than the default to get actual correct behavior (eg, watchers needing require leader to ensure a watcher can't silently lose updates on an etcd leader change). In this case autoRetry=false (correctness) is the default behavior.

The problem with Jepsen tests is that majority of etcd maintainers use go client and it requires non-trivial amount of time to setup jepsen/figure out jetcd differences.

That is a problem, yes. It looks to me like the collective etcd/jetcd community does care about jepsen tests given their visibility, and also given that they do help and have helped in the past to find issues. The jepsen code is closer to the java ecosystem so is easier for jepsen to use a java based client library. It is possible to convert those tests to interface with a go client instead (jepsen has tested systems without a java client, so it is possible). The question is who will invest the time in doing something like that. The argument from jepsen which they made explicit in the closing of their report text in their website is that jetcd is the official java client library for etcd. So from a rational argument perspective I think you (etcd main project) have three options if you want to change this situation:

• Stop endorsing jetcd as an official client. This would require explicitly declaring jetcd as a third party work in progress that is known to have issues. Seems unfair, but is an option.
• Invest in moving the jepsen tests from jetcd to go client
• Invest in making jetcd closer to go client (I recognize that your original PR was an attempt in this direction)

You guys are short on effort and have plenty of other stuff to do, so all the "invest in" above is painful. But then again, so is everybody (short on effort).

Correct me if I'm wrong but your main concern was about

return desc == "there is no address available" || desc == "there is no connection available" 

You are correct, those messages never happen. I've copied from go client for completeness. No need to have them in jetcd

Thanks.