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

Improve Name.Bound documentation. #378

Closed
wants to merge 6 commits into from

Conversation

olix0r
Copy link
Contributor

@olix0r olix0r commented May 5, 2015

Problem

The 'id' and 'path' fields of Name.Bound were insufficiently documented.

Solution

Clarify that 'path' should only contain unbound residual path components and that 'id' should only reflect the bound path.

The 'id' and 'path' fields of Name.Bound were insufficiently documented.

Now the documentation clarifies that 'path' should only contain
unbound residual path components and that 'id' should only reflect
the bound path.
@olix0r
Copy link
Contributor Author

olix0r commented May 5, 2015

Fixes #376

@dschobel
Copy link
Contributor

dschobel commented May 5, 2015

pulling in, thanks for the docs!

@@ -39,7 +39,11 @@ object Name {
*
* Equality of two Names is delegated to `id`. Two Bound instances
* are equal whenever their `id`s are. `id` identifies the `addr`
* and not the `path`.
* and not the `path`. If the `id` is a [[com.twitter.finagle.Name.Path
* Path]], it should only contain *bound*--not residual--path components.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we ever define what residual means?

Copy link
Contributor

Choose a reason for hiding this comment

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

er, to be more precise, I don't know what a residual path is. Can we add a description of what a residual path is, and what it might be used for?

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 was hoping bound and unbound were the operative words and residual was simply an adjective. Basically, given a path like /s/buoyant/proxy/s/svc/app/path/to/resource, a Namer may only bind a prefix like /s/buoyant/proxy and leave the residual path, /s/svc/app/path/to/resource to be bound later (i.e. by the proxy). When the proxy binds the remaining name, it may only bind /s/svc/app, leaving /path/to/resource to be handled by the application in whatever way it deems fit.

I'll try to figure out the best way to fit this into the docs.

@mosesn
Copy link
Contributor

mosesn commented May 5, 2015

For future reference, it would be rad if you included the Problem / Solution / Result in the commit message directly, since we just snarf your commit message in directly with a signed-off-by message.

@olix0r
Copy link
Contributor Author

olix0r commented May 5, 2015

Sorry, it was in there just without the words 'problem' and 'solution'
(otherwise the content was the same). Will keep this in mind next time.

On Tue, May 5, 2015 at 12:48 PM, Moses Nakamura notifications@github.com
wrote:

For future reference, it would be rad if you included the Problem /
Solution / Result in the commit message directly, since we just snarf your
commit message in directly with a signed-off-by message.


Reply to this email directly or view it on GitHub
#378 (comment).

Oliver Gould ver@buoyant.io

@mosesn
Copy link
Contributor

mosesn commented May 5, 2015

No worries!

* Path]], it should only contain *bound*--not residual--path components.
*
* The `path` contains unbound residual path components that were not
* processed duing name resolution.
Copy link
Contributor

Choose a reason for hiding this comment

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

*during

@travisbrown
Copy link
Contributor

Is this ready to go except for the typo, @dschobel, @olix0r? If so I can just pull it in and add the fix before internal review.

@dschobel
Copy link
Contributor

I'd like it better if we included @olix0r's description of residual paths but I think the posted version is still an improvement for experts/people who know the jargon so I'm happy to have it land.

@olix0r
Copy link
Contributor Author

olix0r commented Jun 24, 2015

sorry i haven't popped stack back to this yet. i can address this tomorrow

@olix0r
Copy link
Contributor Author

olix0r commented Jun 24, 2015

I added a note about why residual paths are even a thing. Let me know if it needs more clarification.

@@ -23,6 +23,11 @@ import com.twitter.finagle.util.Showable
* (e.g. `Http.newClient(/s/org/servicename)`). These APIs use `Resolver` under
* the hood to resolve the destination names into the `Name` representation
* of the appropriate cluster.
*
* As names are are bound, a [[com.twitter.finagle.Namer Namer]] may
Copy link
Contributor

Choose a reason for hiding this comment

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

s/are^2/are/

@dschobel
Copy link
Contributor

lgtm modulo typo

@mosesn
Copy link
Contributor

mosesn commented Jun 25, 2015

lgtm

mariusae pushed a commit that referenced this pull request Jun 29, 2015
Problem

The 'id' and 'path' fields of Name.Bound were insufficiently documented.

Solution

Clarify that 'path' should only contain unbound residual path components
and that 'id' should only reflect the bound path.

Github PR #378

Signed-off-by: Daniel Schobel <dschobel@twitter.com>

RB_ID=704664
@luciferous
Copy link
Collaborator

Closed by de6c058

@luciferous luciferous closed this Jul 14, 2015
jbripley pushed a commit to jbripley/finagle that referenced this pull request Oct 28, 2015
Problem

The 'id' and 'path' fields of Name.Bound were insufficiently documented.

Solution

Clarify that 'path' should only contain unbound residual path components
and that 'id' should only reflect the bound path.

Github PR twitter#378

Signed-off-by: Daniel Schobel <dschobel@twitter.com>

RB_ID=704664
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants