You are viewing a plain text version of this content. The canonical link for it is here.
Posted to hdfs-issues@hadoop.apache.org by "Tsz Wo (Nicholas), SZE (JIRA)" <ji...@apache.org> on 2009/08/04 00:45:14 UTC
[jira] Created: (HDFS-524) Further DataTransferProtocol code
refactoring.
Further DataTransferProtocol code refactoring.
----------------------------------------------
Key: HDFS-524
URL: https://issues.apache.org/jira/browse/HDFS-524
Project: Hadoop HDFS
Issue Type: Improvement
Components: data-node
Affects Versions: 0.21.0
Reporter: Tsz Wo (Nicholas), SZE
Assignee: Tsz Wo (Nicholas), SZE
Fix For: 0.21.0
This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Tsz Wo (Nicholas), SZE (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Tsz Wo (Nicholas), SZE updated HDFS-524:
----------------------------------------
Attachment: h524_20090804b.patch
h524_20090804b.patch:
- removed final from private methods.
- added static methods, updateDuration(..) and updateCounter(..) in DataXceiver
- added a new field isLocal in DataXceiver.
- added javadoc for protected methods.
- also changed the fields in DataXceiver to private.
Thanks Cos for the review.
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch, h524_20090804.patch, h524_20090804b.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Tsz Wo (Nicholas), SZE (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Tsz Wo (Nicholas), SZE updated HDFS-524:
----------------------------------------
Hadoop Flags: [Reviewed]
Status: Patch Available (was: Open)
{noformat}
[exec] -1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] -1 tests included. The patch doesn't appear to include any new or modified tests.
[exec] Please justify why no new tests are needed for this patch.
[exec] Also please list what manual steps were performed to verify this patch.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
{noformat}
No new tests added since this is a code refactoring. Running unit tests locally...
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch, h524_20090804.patch, h524_20090804b.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Konstantin Boudnik (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12739266#action_12739266 ]
Konstantin Boudnik commented on HDFS-524:
-----------------------------------------
- {{final}} along with {{private}} seems to be a redundant declaration, 'cause {{private}} modifier assumes that won't be overridden.
- would it make sense to have a separate metric update method to replace nearly identical code like
{noformat}
+ datanode.myMetrics.writeBlockOp.inc(DataNode.now() - opStartTime);
+ final boolean local = s.getInetAddress().equals(s.getLocalAddress());
+ if (local)
+ datanode.myMetrics.writesFromLocalClient.inc();
+ else
+ datanode.myMetrics.writesFromRemoteClient.inc();
{noformat}
which would handle and wrap the update logic? Perhaps {{updateDNMetrics(MetricsBase metricType)}} or similar?
- also,
{noformat}
+ final boolean local = s.getInetAddress().equals(s.getLocalAddress());
{noformat}
might be converted to the class level constant to avoid an extra initialization on each call of {{opWriteBlock}} and {{opReadBlock}}
I know that only public methods should have JavaDoc, but wouldn't it be good to have them for protected ones as well? :-)
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch, h524_20090804.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Hudson (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12739459#action_12739459 ]
Hudson commented on HDFS-524:
-----------------------------
Integrated in Hadoop-Hdfs-trunk #40 (See [http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/40/])
. Further DataTransferProtocol code refactoring.
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch, h524_20090804.patch, h524_20090804b.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Konstantin Boudnik (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12739192#action_12739192 ]
Konstantin Boudnik commented on HDFS-524:
-----------------------------------------
As I've suspected it was my ignorance talking - not me :-)
After some more careful reading of the code, it turned out to me that
- while {{Op}} might be a good candidate to become a factory which will be handling instantiation of all these new operation specific classes
- {{DataXceiver}} might not need to extend {{DataTransferProtocol.Receiver}} as soon as all operations are pulled out into a separate classes
it won't solve all the problems.
The biggest one, in my opinion, is that {{DataTransferProtocol.Receiver}} has two inner classes {{Sender}} and {{Receiver}} which have their own sets of unique operations to read, write, and copy blocks. Although, it might be possible to generalize those with some combination of polymorphism and delegation, it won't improve the current situation and will, perhaps, degrade the existing design.
I guess I'm ready to withdraw my initial comment - thanks for crashing it :-) !
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Tsz Wo (Nicholas), SZE (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Tsz Wo (Nicholas), SZE updated HDFS-524:
----------------------------------------
Attachment: h524_20090804.patch
I agree that the current Sender and Receiver classes aren't the best design. Since we probably will re-implement everything with Avro in the future, let's keep the changes minimal.
h524_20090804.patch: changed public methods in DataXceiver to protected.
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch, h524_20090804.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Konstantin Boudnik (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12739299#action_12739299 ]
Konstantin Boudnik commented on HDFS-524:
-----------------------------------------
+1 patch looks good!
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch, h524_20090804.patch, h524_20090804b.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Tsz Wo (Nicholas), SZE (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12739139#action_12739139 ]
Tsz Wo (Nicholas), SZE commented on HDFS-524:
---------------------------------------------
bq. in DataXceiver: is datanode.getXceiverCount() is so costly that it really make sense performance wise?
getXceiverCount() alone is not costly. However, constructing the string, which creates intermediate objects, inside LOG.debug(..) is unnecessary. Since we often got gc problem, I think it is good to minimize object creations.
{quote}
it seems that the switch statement won't grow up anytime soon, but wouldn't it be better looking and perhaps more efficient to replace it with polymorphism? It will increase a number of classes though, but the control flow would be better enclosed within new classes. I.e. the operations like opReadBlock(), opBlockChecksum(), etc. could be moved to a smaller classes like OpBlockRead, OpBlockCheckSum which will expose only one public method, say, doOperation().
{quote}
This is a good idea. Let's discuss class structure in more details.
In the patch, we have
{code}
//DataTransferProtocol.java
public enum Op {
WRITE_BLOCK((byte)80),
...
}
public static abstract class Receiver {
protected final void processOp(Op op, DataInputStream in
) throws IOException {
switch(op) {
case WRITE_BLOCK:
opWriteBlock(in);
break;
...
}
private final void opWriteBlock(DataInputStream in) throws IOException {
final long blockId = in.readLong();
...
opWriteBlock(in, blockId, blockGs, pipelineSize, isRecovery,
client, src, targets, accesstoken);
}
protected abstract void opWriteBlock(DataInputStream in,
long blockId, long blockGs, int pipelineSize, boolean isRecovery,
String client, DatanodeInfo src, DatanodeInfo[] targets,
AccessToken accesstoken) throws IOException;
}
}
{code}
In the implementation, DataXceiver extends DataTransferProtocol.Receiver and implements all the abstract methods.
{code}
//DataXceiver.java
class DataXceiver extends DataTransferProtocol.Receiver
implements Runnable, FSConstants {
...
public void opWriteBlock(DataInputStream in, long blockId, long blockGs,
int pipelineSize, boolean isRecovery,
String client, DatanodeInfo srcDataNode, DatanodeInfo[] targets,
AccessToken accessToken) throws IOException {
...
}
}
{code}
If we have the proposed classes OpBlockRead, OpBlockCheckSum, etc., then how should DataXceiver implement the abstract methods?
Also, how to relate the new classes OpBlockRead and OpBlockCheckSum with the enum Op?
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Tsz Wo (Nicholas), SZE (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Tsz Wo (Nicholas), SZE updated HDFS-524:
----------------------------------------
Resolution: Fixed
Status: Resolved (was: Patch Available)
Passed all unit tests.
I have committed this.
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch, h524_20090804.patch, h524_20090804b.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Konstantin Boudnik (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12738995#action_12738995 ]
Konstantin Boudnik commented on HDFS-524:
-----------------------------------------
A couple of comments (perhaps because of my ignorance):
- in DataXceiver: is datanode.getXceiverCount() is so costly that it really make sense performance wise?
{noformat}
- LOG.debug(datanode.dnRegistration + ":Number of active connections is: "
- + datanode.getXceiverCount());
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(datanode.dnRegistration + ":Number of active connections is: "
+ + datanode.getXceiverCount());
+ }
{noformat}
- it seems that the switch statement won't grow up anytime soon, but wouldn't it be better looking and perhaps more efficient to replace it with polymorphism? It will increase a number of classes though, but the control flow would be better enclosed within new classes. I.e. the operations like opReadBlock(), opBlockChecksum(), etc. could be moved to a smaller classes like OpBlockRead, OpBlockCheckSum which will expose only one public method, say, doOperation().
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HDFS-524) Further DataTransferProtocol code
refactoring.
Posted by "Tsz Wo (Nicholas), SZE (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HDFS-524?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Tsz Wo (Nicholas), SZE updated HDFS-524:
----------------------------------------
Attachment: h524_20090803.patch
h524_20090803.patch:
- In DataTransferProtocol.Receiver,
-* renamed op(..) to readOp(..),
-* changed the public opXxxx(DataInputStream in) methods to private, and
-* changed the public abstract methods to protected.
- Moved the switch-case statement from DataXceiver to DataTransferProtocol.Receiver.processOp(..).
Note that there is no incompatible change since the DataTransferProtocol.Receiver APIs are not yet released.
> Further DataTransferProtocol code refactoring.
> ----------------------------------------------
>
> Key: HDFS-524
> URL: https://issues.apache.org/jira/browse/HDFS-524
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node
> Affects Versions: 0.21.0
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: 0.21.0
>
> Attachments: h524_20090803.patch
>
>
> This is a further refactoring over HDFS-377 to move generic codes from DataXceiver to DataTransferProtocol.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.