-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Update transport version docs #133862
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
Update transport version docs #133862
Conversation
🔍 Preview links for changed docs |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Pinging @elastic/core-docs (Team:Docs) |
ldematte
left a comment
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 know it's probably early for a full review, as the section on details/description is still missing, but what is here so far looks clear and easy to follow to me.
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 think this would benefit from a brief overview of the principles behind the TV allocation system, like what files there are, what they mean, and what the system invariants are. Then we can dive into the operational recipes.
docs/internal/Versioning.md
Outdated
| args. Because this command is idempotent, it will detect that the transport version constant is no longer referenced, | ||
| and remove the corresponding state file. E.g.: |
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'd avoid this reference to idempotence here. Idempotence just means if you run it twice, it has the same effect as running it once, and that has no bearing on this behaviour. (I think you might be thinking more of "history independence" rather than idempotence here. They're related but not identical.)
As a user, I'd have the following question: suppose I merge a new TV, then in another PR I remove all references to that TV. Could that TV then be reused for someone else's PR?
docs/internal/Versioning.md
Outdated
| To prevent race conditions, all transport version ids are first created on the `main` branch, after which they can be | ||
| backported to prior release branches. |
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.
This term "race condition" is doing a lot of heavy lifting here. Perhaps something like this:
To prevent concurrently developed PRs from allocating the same transport version ID, ...
docs/internal/Versioning.md
Outdated
|
|
||
| When in doubt, you can always leave the code as-is. | ||
| This is an optional cleanup step that is never required for correctness. | ||
| Backport state files can either be generated on the command line by gradle, or by adding a gihub version label to your |
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.
This is the first mention of "backport state files". We should probably define this term.
docs/internal/Versioning.md
Outdated
| Backport state files can either be generated on the command line by gradle, or by adding a gihub version label to your | ||
| PR. | ||
|
|
||
| If you have already generated a transport version file, gradle will not be able to automatically detect which transport |
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.
If I'm not mistaken, this is describing a more unusual / exceptional situation. We should start by describing how a normal "canonical" backport works, and then this description will make more sense by describing the differences.
docs/internal/Versioning.md
Outdated
|
|
||
| ./gradlew :generateTransportVersion --name=my_new_tv --backport-branches=9.1,9.0 | ||
|
|
||
| If you later decide that you'd like to remove a backport, you can rerun the above command without the branch you want to |
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.
How much later? This stops working once the main PR is merged, right?
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.
And all of this happens on the main branch, correct? If so, I think the part where we describe how to actually do backports to other branches is missing.
docs/internal/Versioning.md
Outdated
| `transport/upper_bounds/` resource directory. To resolve the conflict, rerun the full generation command, and it will | ||
| regenerate the state files, including the new transport version id. E.g.: | ||
|
|
||
| ./gradlew :generateTransportVersion --name=my_new_tv |
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.
What happens to backports in this case?
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
|
@prdoyle I've just pushed a rework of most of these new docs. We want these to be developer focused, not get bogged down in details about how the new system works. For details we will heavily document the new system in TransportVersion.java. But here we want the docs to focus on how developers will interact with the new system. Given the new changes I pushed, and the direction we are going for, can you take another look? |
|
The most recent version of this looks really good, imho very intuitive and clear for someone who hasn't been involved in the heavy details of this. This is awesome and so much simpler! |
ldematte
left a comment
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.
Two nits, but it is very easy to understand!
I think this is a great step ahead and it makes using and backporting TV very straightforward
prdoyle
left a comment
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'd recommend at least briefly describing how TVs are managed now: what the files are, what they contain, and what we do with them.
Having at least a broad mental model of what's going on can be really helpful to give devs an intuition for what command to run when.
| You can also let CI handle updating transport versions for you. As version | ||
| labels are updated on your PR, the generation task is automatically run with | ||
| the appropriate backport branches and any changes to the internal state files | ||
| are committed to your branch. |
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.
As a developer, my eyes would glaze over as soon as I see "let CI handle updating transport versions for you" and come away with the message that this is all automated and I don't need to keep reading.
If that's not the case, we should probably add a few words on the situations where this works and where it doesn't.
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 think that's pretty much the takeaway we want. CI will "just work" in most cases. The only scenario in which you'd not just rely on CI to do it for you is if you had to verify these changes locally, in which case you'd have to apply the change manually, or let CI do it, and pull the automated change.
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.
Ok that is great. In that case we might have buried the lede here a little. I wonder if we could call this out as a TL;DR somewhere prominent.
| your transport version `my_tv` to `main` and `9.1`, and then realized you also | ||
| needed to backport to `8.19` you would run (in `main`): | ||
|
|
||
| ./gradlew generateTransportVersion --name=my_tv --backport-branches=9.1,8.19 |
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 see the value of making backport-branches the ultimate authority on backports; but also, in the specific user story you're describing here, that's counterproductive and error prone. (For example, suppose they failed to notice that 9.0 should also have been included.)
As a user, I'd like to say something like --add-backport-branches=8.19, or --backport-branches=+8.19, and know that it's not going to nuke any existing ones. (FWIW this is also idempotent.)
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 think this is a fair point, but I would like to start without this, and add it in the future (it's a bit of complexity to the task implementation). The vast majority of the time developers can just let CI regenerate the transport version based on version labels.
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.
Works for me!
|
@prdoyle I added a section on the layout of the resources files at a high level, hopefully this gives the overview you were hoping for (and it helped define "internal state files" which we use in several parts of this change). |
| ### Internal state files | ||
|
|
||
| The Elasticsearch server jar contains resource files representing each | ||
| transport version. These files are loaded at runtime to construct | ||
| `TransportVersion` instances. Since each transport version has its own file | ||
| they can be backported without conflict. | ||
|
|
||
| Additional resource files represent the latest transport version known on | ||
| each release branch. If two transport versions are added at the same time, | ||
| there will be a conflict in these internal state files, forcing one to be | ||
| regenerated to resolve the conflict before merging to `main`. | ||
|
|
||
| All of these internal state files are managed by gradle tasks; they should | ||
| not be edited directly. |
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.
This is perfect. It's exactly what I had in mind.
Updates the docs for the new Transport Version model. This should only be merged once we are aready for this new model to be generally used.