Skip to content

[SPARK-21749][DOC] Add comments for MessageEncoder to explain the wire format#18965

Closed
neoremind wants to merge 1 commit intoapache:masterfrom
neoremind:add_comments
Closed

[SPARK-21749][DOC] Add comments for MessageEncoder to explain the wire format#18965
neoremind wants to merge 1 commit intoapache:masterfrom
neoremind:add_comments

Conversation

@neoremind
Copy link

What changes were proposed in this pull request?

Spark RPC is built upon TCP tier and leverage netty framework. It would be better to have some comments in RPC module especially to explain the RPC message wire format since it is critical to understand what Message consists (header + payload). So Javadoc comments can be added into MessageEncoder class.

How was this patch tested?

Only comments added. Existing unit tests are all passed.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@neoremind
Copy link
Author

@jerryshao please review my separated PR. Thanks!

@jiangxb1987
Copy link
Contributor

Seems you should add the [DOC] tag in your title.

@neoremind neoremind changed the title [SPARK-21749][CORE] Add comments for MessageEncoder to explain the wire format [SPARK-21749][DOC] Add comments for MessageEncoder to explain the wire format Aug 17, 2017
@jerryshao
Copy link
Contributor

We usually don't have the PRs which only add comments to explain something, so I'm neutral to this change.

@neoremind
Copy link
Author

neoremind commented Aug 17, 2017

I see, anyway this is what I found when I dig into the wire protocol of spark rpc since wire format is a big part for understanding the message structure. If someone thinks this is not necessary I can close the PR.

@srowen
Copy link
Member

srowen commented Aug 17, 2017

I think the more important question is, is it a protocol that we are guaranteeing? not sure that is. If not then I don't think it should be in user-facing docs.

@neoremind
Copy link
Author

@srowen I see your concern, it is more internal oriented and maybe updated by developer as lib evolves, I will close the PR then. Thanks for reviewing!

@neoremind neoremind closed this Aug 17, 2017
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