hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aaron T. Myers (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-6940) Initial refactoring to allow ConsensusNode implementation
Date Mon, 08 Sep 2014 19:44:29 GMT

    [ 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}}:

-  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 @@
-  private void startSecretManager() {
+  protected void startSecretManager() {
     if (dtSecretManager != null) {
       try {
@@ -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() ||
@@ -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();
@@ -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();
     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) {
     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 {

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

View raw message