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

Reorganized, renamed, and moved the Technical Overview information (p… #843

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dwelsch-esi
Copy link
Contributor

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".

…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]>
Copy link
Member

@jmhbnz jmhbnz left a 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.

@dwelsch-esi
Copy link
Contributor Author

@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.

@ivanvc
Copy link
Member

ivanvc commented May 14, 2024

Thanks for the PR, @dwelsch-esi. I briefly looked into it but felt overwhelmed, as there were too many changes. I would suggest:

  1. Break down the PR into more atomic ones; that way, it will be better for them to merge quicker and lessen the burden on the reviewers.
  2. I agree with @jmhbnz. It may be confusing if the documents are not there when switching versions. One approach would be to add redirections to the new locations rather than just moving the files.

Copy link
Contributor

@nate-double-u nate-double-u left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

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.)*
Copy link
Contributor

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.

Copy link
Contributor Author

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.)*
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment above.

Copy link
Contributor Author

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
```
Copy link
Contributor

Choose a reason for hiding this comment

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

GitHub is rendering this section weirdly for me -- showing things have been changed above the fold. See image for what I mean. Let's double check that the changes reflect your intention.

Screenshot 2024-05-17 at 2 32 04 PM

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming.

Copy link
Contributor

@nate-double-u nate-double-u left a comment

Choose a reason for hiding this comment

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

/lgtm

@jberkus
Copy link
Contributor

jberkus commented Aug 26, 2024

I'll shepherd this in but I need to fix the merge conflicts first.

@jmhbnz
Copy link
Member

jmhbnz commented Aug 26, 2024

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.

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.

5 participants