[
https://issues.apache.org/jira/browse/HDFS-6940?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14125979#comment-14125979
]
Aaron T. Myers commented on HDFS-6940:
--------------------------------------
bq. Suresh, if you want to judge peoples' respectfulness, you should probably
look at your own style of communication. Your subjective opinions on the value
of people contributions are incomplete and hostile. Please consider apology.
I 100% agree with [~sureshms] on this. It is a well-known expectation that
those who are not familiar with certain areas of the codebase refrain from
+1'ing patches to that area of the code base. It's not about the value of the
contributions - it's about the areas of expertise. That's just common sense,
not subjective judgment. This is the reason that we have discussed in the past
having component areas and component owners. Up until now I have opposed that,
because I thought that the existing understanding was sufficient. This JIRA and
others may be demonstrating that we may need some actual rules on this.
bq. Aaron, you never commented or objected on the patch itself. My
understanding that you are questioning the implementation of ConsensusNode via
sub-classing the NameNode. You actually posted your reservations even before
the patch was submitted.
That's correct, I posted objections to the entire direction of the patch. I
could go through line by line if you'd like, but that's not usually necessary
when one objects to the patch as a whole. It's difficult for me to understand
how the comments that I posted could be interpreted to mean that I was OK with
committing the patch. At the very least, I don't understand how this could be
misinterpreted: "Anyway, I'm fine if you want to proceed with this direction,
but please only commit this to the branch, not to trunk." You then committed it
to trunk and branch-2 without any further discussion of that subject. For this
reason alone this patch should not have been committed, and should be reverted.
bq. I unswered this already. Will elaborate. Working on a branch means a lot of
merging from trunk to the branch intended to keep the bunch in sync with trunk.
So it used to be a general practice in this community to make initial
refactoring on the trunk, if such changes keep the trunk in working condition,
in order to easy those frequent merges. Found this conversation, for example.
That's generally only the case when the changes are in and of themselves good
changes to make as well. That is not the case in the case of this JIRA, where
opening up method visibility is not independently a good idea. At the very
least, it doesn't make any sense to have committed this to branch-2 in
_addition_ to trunk. Why was that done?
bq. I did create a jira to discuss ConsensusNode interfaces including your
topic HDFS-7007. And this jira has nothing to do with NameNode subclassing. At
all. I did ask explicitly if you have technical objections to the patch, and
since you did not reply I assumed you don't have any.
The patch is 100% about subclassing. The bulk of the patch is about opening up
the visibility of certain methods to allow subclassing, for example all of
these changes to {{FSNamesystem}}:
{code}
- private void loadFSImage(StartupOption startOpt) throws IOException {
+ protected void loadFSImage(StartupOption startOpt) throws IOException {
final FSImage fsImage = getFSImage();
// format before starting up if requested
@@ -1036,7 +1036,7 @@
imageLoadComplete();
}
- private void startSecretManager() {
+ protected void startSecretManager() {
if (dtSecretManager != null) {
try {
dtSecretManager.startThreads();
@@ -1048,7 +1048,7 @@
}
}
- private void startSecretManagerIfNecessary() {
+ protected void startSecretManagerIfNecessary() {
boolean shouldRun = shouldUseDelegationTokens() &&
!isInSafeMode() && getEditLog().isOpenForWrite();
boolean running = dtSecretManager.isRunning();
@@ -1198,7 +1198,7 @@
return haEnabled && inActiveState() && startingActiveService;
}
- private boolean shouldUseDelegationTokens() {
+ protected boolean shouldUseDelegationTokens() {
return UserGroupInformation.isSecurityEnabled() ||
alwaysUseDelegationTokensForTests;
}
@@ -2728,6 +2728,7 @@
* @throws UnresolvedLinkException
* @throws IOException
*/
+ protected
LocatedBlock prepareFileForWrite(String src, INodeFile file,
String leaseHolder, String clientMachine,
boolean writeToEditLog,
@@ -3184,6 +3185,7 @@
return new FileState(pendingFile, src);
}
+ protected
LocatedBlock makeLocatedBlock(Block blk, DatanodeStorageInfo[] locs,
long offset) throws IOException {
LocatedBlock lBlk = new LocatedBlock(
@@ -3301,8 +3303,8 @@
return true;
}
- private INodeFile checkLease(String src, String holder, INode inode,
- long fileId)
+ protected INodeFile checkLease(String src, String holder, INode inode,
+ long fileId)
throws LeaseExpiredException, FileNotFoundException {
assert hasReadLock();
final String ident = src + " (inode " + fileId + ")";
@@ -4412,7 +4414,7 @@
return leaseManager.reassignLease(lease, src, newHolder);
}
- private void commitOrCompleteLastBlock(final INodeFile fileINode,
+ protected void commitOrCompleteLastBlock(final INodeFile fileINode,
final Block commitBlock) throws IOException {
assert hasWriteLock();
Preconditions.checkArgument(fileINode.isUnderConstruction());
@@ -4808,6 +4810,7 @@
* @return an array of datanode commands
* @throws IOException
*/
+ protected
HeartbeatResponse handleHeartbeat(DatanodeRegistration nodeReg,
StorageReport[] reports, long cacheCapacity, long cacheUsed,
int xceiverCount, int xmitsInProgress, int failedVolumes)
@@ -4857,8 +4860,8 @@
* @param file
* @param logRetryCache
*/
- private void persistBlocks(String path, INodeFile file,
- boolean logRetryCache) {
+ protected void persistBlocks(String path, INodeFile file,
+ boolean logRetryCache) {
assert hasWriteLock();
Preconditions.checkArgument(file.isUnderConstruction());
getEditLog().logUpdateBlocks(path, file, logRetryCache);
@@ -5289,7 +5292,7 @@
* @param path
* @param file
*/
- private void persistNewBlock(String path, INodeFile file) {
+ protected void persistNewBlock(String path, INodeFile file) {
Preconditions.checkArgument(file.isUnderConstruction());
getEditLog().logAddBlock(path, file);
if (NameNode.stateChangeLog.isDebugEnabled()) {
@@ -7167,7 +7170,7 @@
*
* @return true if delegation token operation is allowed
*/
- private boolean isAllowedDelegationTokenOp() throws IOException {
+ protected boolean isAllowedDelegationTokenOp() throws IOException {
{code}
Changing those visibilities is not a useful refactor in and of itself, and in
my opinion is on its own a poor decision to make. What reason is there to
change those method visibilities except to support subclassing?
I also fail to see how changing these on trunk/branch-2 will make merges any
easier. Could you explain that?
bq. I don't think this JIRA is appropriate to this kind of discussion...
At this point I agree that this JIRA has gone off the rails. Multiple issues
are now being discussed that are not specific to this JIRA. I believe that for
the time being we should revert this change from trunk/branch-2 until
discussion on that is finished. Separately, we should continue some of these
discussions elsewhere. I'll be bringing this JIRA to the attention of the
Hadoop PMC, as it has gotten out of hand.
> Initial refactoring to allow ConsensusNode implementation
> ---------------------------------------------------------
>
> Key: HDFS-6940
> URL: https://issues.apache.org/jira/browse/HDFS-6940
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode
> Affects Versions: 2.0.6-alpha, 2.5.0
> Reporter: Konstantin Shvachko
> Assignee: Konstantin Shvachko
> Fix For: 2.6.0
>
> Attachments: HDFS-6940.patch
>
>
> Minor refactoring of FSNamesystem to open private methods that are needed for
> CNode implementation.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
