HADOOP-18981. moved oncrpc/portmap from hadoop-common-project/hadoop-nfs to hadoop-common-project/hadoop-common #6280
Conversation
…common-project/hadoop-common
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
If I understand correctly you just need Moving files and the widespread whitespace changes will make future small backports difficult, unless this commit is also backported. |
|
The key point is if we use UDPClient/server from other module, then it becomes inappropriate to put this under hadoop-common-project/hadoop-nfs module. And if we don't fix it now but build upon this, it would make code harder to maintain and would take more work if we want to fix it in the future. Another example is the key-value Interface/implementation. Right now, YARN uses a key-value store to store some persistent data and they put key-value interface/implementation in YARN module. However, we may look into store persistent k/v pairs too for HDFS. We should move key-value interface/implemention into hadoop-common, rather than adding a dependency on YARN from HDFS. I still remember a lesson I learned from reading John Ousterhout's "a philosophy of software engineering" book: when we make a change to an existing system, we should make the new change such that the new change seems to be designed from the beginning. On the other hand, most of us tend to try minimizing line of changes, which ideally shouldn't not be a concern. |
|
@simbadzina / @goiri / @ZanderXu / @Hexiaoqiao Can i get a review? What do you guys think of this PR? Thanks, |
|
The PR looks ok to me. My only concern is the number of files that are moved and changed. I'll wait for the other to chime in. Making this jira as a subtask of that one may provide better context.The refactor makes sense and I agree it is better to do it sooner. |
|
thanks @simbadzina for providing your comments. I created HDFS-17286 and linked it to this jira. |
|
@Hexiaoqiao, @goiri, @ZanderXu, thoughts? Could we move forward? |
|
@xinglin @simbadzina Thanks for your works. +1 from my side. The only concern is that is it compatible changes to end user if dependency with oncrpc/portmap? Thanks. |
That is a good point: previously, whoever depends on oncrpc/portmap would have a dependency on hadoop-nfs module. With this relocation, they need to change dependency to be hadoop-common module. This should be the only change, as we did not rename package names/paths. |
It's OK for me if only check in branch-3.4, and we should mark 'Incompatible changes' flag when check in. Thanks. |
|
That makes sense. Thanks for reviewing and offering advice! |
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
LGTM
I wouldn't even consider it as incompatible...it's just moving classes around.
one thing I would propose is for the new packages to have package-info.java files describing the modules as @Private .
|
Note that as hadoop-common is a provided dependency of hadoop-nfs, with explicit use of |
|
@steveloughran, Thanks for taking a look at this change. I added a couple of package-info.java files. First time doing this and not sure whether I am adding these files correctly. Could you take a look? |
If this is the case, I guess we can be assured that hadoop-common is always available whenever hadoop-nfs is needed/depended. And moving classes around as done by this PR won't break these other modules which used to be depended on hadoop-nfs module. Am i understanding it correctly? |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran / @Hexiaoqiao please help move this PR forward. thanks, |
|
I am +1, having been probably the last person maintaining the NFS code myself. Had a quick pass of the PR. Seems fine. On a side note, hadoop-common is used by many many applications, even indirectly. It's likely anything you add to hadoop-common implicitly makes countless applications bloated unfortunately. I don't really care now given how powerful bare metal machines are, but someone in the kubernetes or docker world might complain. I see that the PR doesn't introduce additional dependencies in pom.xml so it's good. |
|
Glad to see it got merged! Thanks for everyone for your reviews! |
…he#6280) Move the org.apache.hadoop.{oncrpc, portmap} packages from the hadoop-nfs module to the hadoop-common module. This allows for use of the protocol beyond just NFS -including within HDFS itself. Contributed by Xing Lin
6 participants
Description of PR
We want to use udpserver/client for other use cases, rather than only for NFS. One such use case is to export NameNodeHAState for NameNodes via a UDP server.
How was this patch tested?
First commit: rename from hadoop-nfs to hadoop-common for oncrpc and portmap dir.
Second commit: removed end of line spaces
wait for jenkins build results.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?