-
Notifications
You must be signed in to change notification settings - Fork 296
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
Reorganized, renamed, and moved the Technical Overview information (p… #843
base: main
Are you sure you want to change the base?
Conversation
…reviously "Learning") per: etcd-io#771 (comment) Did some minor rewriting. Moved the Glossary out of "Learning" into the main table of contents after "FAQ". Signed-off-by: David Welsch <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for proposing this @dwelsch-esi. The first question in my mind was given this is quite a substantial restructure is it something we should apply to all supported docs versions at the same time? I.e. include v3.4
and v3.5
?
My concern would be users getting confused moving between docs for say v3.5
and v3.6
etc.
Curious to hear what the community thinks on this.
@jmhbnz Balance that against confusing users who are familiar with the current documentation. My take would be to treat it like a software update: If it's a bug fix, it might be appropriate to also apply it to existing versions, but since it's (essentially) an enhancement, put it in the new release only. There might be considerations I haven't thought of, though. If the project votes to apply it to other versions, I'm happy to do so. |
Thanks for the PR, @dwelsch-esi. I briefly looked into it but felt overwhelmed, as there were too many changes. I would suggest:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of a feature update or bug fix, I agree we should update all the affected versions; however, here we're doing a reorg, and should probably only update next version (3.6). The draw back is maintaining future 3.5 updates that would need to be ported to 3.6 as the writers will need to know where things shifted.
As for this update, I don't think it's too big a PR, there's just a lot of moved files.
Some comments in line, otherwise LGTM
/lgtm
|
||
Both KV and Watch APIs allow access to not only the latest versions of keys, but | ||
also previous versions are accessible within a continuous history window, limited | ||
Both the KV and Watch APIs allow access to the latest versions of keys, plus all previous versions within a continuous history window limited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Often times we use short lines (~80 chars or so) to help localization teams to make their updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored shorter line lengths in this file.
@@ -138,3 +132,5 @@ Client-side keepalive ping still does not reason about network partitions. Strea | |||
![client-balancer-figure-07.png](../img/client-balancer-figure-07.png) | |||
|
|||
Currently, retry logic is handled manually as an interceptor. This may be simplified via [official gRPC retries](https://github.com/grpc/proposal/blob/master/A6-client-retries.md). | |||
|
|||
*Authors: Gyuho Lee (github.com/gyuho, Amazon Web Services, Inc.), Joe Betz (github.com/jpbetz, Google Inc.)* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we still need author attribution here? This isn't a blog post, and I think that the commit history would show the file authorship.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed authorship lines.
@@ -128,3 +123,5 @@ Reference | |||
- Use case: [etcd#3715](https://github.com/etcd-io/etcd/issues/3715) | |||
- Use case: [etcd#8888](https://github.com/etcd-io/etcd/issues/8888) | |||
- Use case: [etcd#10114](https://github.com/etcd-io/etcd/issues/10114) | |||
|
|||
*Authors: Gyuho Lee (github.com/gyuho, Amazon Web Services, Inc.), Joe Betz (github.com/jpbetz, Google Inc.)* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed authorship lines.
@@ -399,7 +399,7 @@ _, err := kvc.Get(ctx, "a") | |||
+ if s.Code() == codes.Canceled | |||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code example starting on line 383 is a file diff illustrating the differences between the 3.2 and 3.3 config files. GitHub is interpreting the diff notation and highlighting the added and removed lines. GitHub is confused, since these removes/adds are not actually changes to the repo file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming.
Signed-off-by: David Welsch <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
I'll shepherd this in but I need to fix the merge conflicts first. |
I'm not sure we have consensus for this to proceed as is. I would like to see the concern addressed about compatibility of docs between versions. Ivan had a great suggestion in #843 (comment) of using redirects to ensure links work across versions. |
Updated, reorganized,and renamed the technical overview (previously "Learning") per:
#771 (comment)
Did some minor rewriting. Moved the Glossary out of "Learning" into the main table of contents after "FAQ".