Activity

-------------------------------------------------------------------------------
Test set: org.apache.hadoop.hdfs.TestDfsOverAvroRpc
-------------------------------------------------------------------------------
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.486 sec <<< FAILURE!
testWorkingDirectory(org.apache.hadoop.hdfs.TestDfsOverAvroRpc) Time elapsed: 1.424 sec <<< ERROR!
org.apache.avro.AvroTypeException: Two methods with same name: delete
at org.apache.avro.reflect.ReflectData.getProtocol(ReflectData.java:394)
at org.apache.avro.ipc.reflect.ReflectResponder.<init>(ReflectResponder.java:36)
at org.apache.hadoop.ipc.AvroRpcEngine.createResponder(AvroRpcEngine.java:189)
at org.apache.hadoop.ipc.AvroRpcEngine$TunnelResponder.<init>(AvroRpcEngine.java:196)
at org.apache.hadoop.ipc.AvroRpcEngine.getServer(AvroRpcEngine.java:232)
at org.apache.hadoop.ipc.RPC.getServer(RPC.java:550)
at org.apache.hadoop.hdfs.server.namenode.NameNode.initialize(NameNode.java:432)
at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:567)
at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:559)
at org.apache.hadoop.hdfs.server.namenode.NameNode.createNameNode(NameNode.java:1546)
at org.apache.hadoop.hdfs.MiniDFSCluster.createNameNode(MiniDFSCluster.java:637)
at org.apache.hadoop.hdfs.MiniDFSCluster.initMiniDFSCluster(MiniDFSCluster.java:541)
at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:257)
at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:85)
at org.apache.hadoop.hdfs.MiniDFSCluster$Builder.build(MiniDFSCluster.java:243)
at org.apache.hadoop.hdfs.TestLocalDFS.testWorkingDirectory(TestLocalDFS.java:64)
at org.apache.hadoop.hdfs.TestDfsOverAvroRpc.testWorkingDirectory(TestDfsOverAvroRpc.java:30)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at junit.framework.TestCase.runTest(TestCase.java:168)
at junit.framework.TestCase.runBare(TestCase.java:134)
at junit.framework.TestResult$1.protect(TestResult.java:110)
at junit.framework.TestResult.runProtected(TestResult.java:128)
at junit.framework.TestResult.run(TestResult.java:113)
at junit.framework.TestCase.run(TestCase.java:124)
at junit.framework.TestSuite.runTest(TestSuite.java:232)
at junit.framework.TestSuite.run(TestSuite.java:227)
at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:83)
at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:59)
at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.executeTestSet(AbstractDirectoryTestSuite.java:120)
at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.execute(AbstractDirectoryTestSuite.java:145)
at org.apache.maven.surefire.Surefire.run(Surefire.java:104)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at org.apache.maven.surefire.booter.SurefireBooter.runSuitesInProcess(SurefireBooter.java:290)
at org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:1017)

Aaron T. Myers
added a comment - 29/Aug/11 18:42 The full stack trace:
{nformat}
-------------------------------------------------------------------------------
Test set: org.apache.hadoop.hdfs.TestDfsOverAvroRpc
-------------------------------------------------------------------------------
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.486 sec <<< FAILURE!
testWorkingDirectory(org.apache.hadoop.hdfs.TestDfsOverAvroRpc) Time elapsed: 1.424 sec <<< ERROR!
org.apache.avro.AvroTypeException: Two methods with same name: delete
at org.apache.avro.reflect.ReflectData.getProtocol(ReflectData.java:394)
at org.apache.avro.ipc.reflect.ReflectResponder.<init>(ReflectResponder.java:36)
at org.apache.hadoop.ipc.AvroRpcEngine.createResponder(AvroRpcEngine.java:189)
at org.apache.hadoop.ipc.AvroRpcEngine$TunnelResponder.<init>(AvroRpcEngine.java:196)
at org.apache.hadoop.ipc.AvroRpcEngine.getServer(AvroRpcEngine.java:232)
at org.apache.hadoop.ipc.RPC.getServer(RPC.java:550)
at org.apache.hadoop.hdfs.server.namenode.NameNode.initialize(NameNode.java:432)
at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:567)
at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:559)
at org.apache.hadoop.hdfs.server.namenode.NameNode.createNameNode(NameNode.java:1546)
at org.apache.hadoop.hdfs.MiniDFSCluster.createNameNode(MiniDFSCluster.java:637)
at org.apache.hadoop.hdfs.MiniDFSCluster.initMiniDFSCluster(MiniDFSCluster.java:541)
at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:257)
at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:85)
at org.apache.hadoop.hdfs.MiniDFSCluster$Builder.build(MiniDFSCluster.java:243)
at org.apache.hadoop.hdfs.TestLocalDFS.testWorkingDirectory(TestLocalDFS.java:64)
at org.apache.hadoop.hdfs.TestDfsOverAvroRpc.testWorkingDirectory(TestDfsOverAvroRpc.java:30)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at junit.framework.TestCase.runTest(TestCase.java:168)
at junit.framework.TestCase.runBare(TestCase.java:134)
at junit.framework.TestResult$1.protect(TestResult.java:110)
at junit.framework.TestResult.runProtected(TestResult.java:128)
at junit.framework.TestResult.run(TestResult.java:113)
at junit.framework.TestCase.run(TestCase.java:124)
at junit.framework.TestSuite.runTest(TestSuite.java:232)
at junit.framework.TestSuite.run(TestSuite.java:227)
at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:83)
at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:59)
at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.executeTestSet(AbstractDirectoryTestSuite.java:120)
at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.execute(AbstractDirectoryTestSuite.java:145)
at org.apache.maven.surefire.Surefire.run(Surefire.java:104)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at org.apache.maven.surefire.booter.SurefireBooter.runSuitesInProcess(SurefireBooter.java:290)
at org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:1017)

Uma Maheswara Rao G
added a comment - 30/Aug/11 11:40 Hi Aaron,
Looks Avro does not support multiple APIs with same name
http://web.archiveorange.com/archive/v/du1upKgQKN4KYfuW7tng
But we have the cases in NameNodeProtocols.
public interface NamenodeProtocols
extends ClientProtocol,
DatanodeProtocol,
NamenodeProtocol,
RefreshAuthorizationPolicyProtocol,
RefreshUserMappingsProtocol,
GetUserMappingsProtocol {
One more observation is that reportedBadBlocks, versionRequest and etc.. present in ClientProtocol, DatanodeProtocol. I think this case also will not be allowed.
Since we have such scenarios to support, we may need to check with Avro.
we can file one JIRA in AVRO or you have some workaround for this?
Thanks
Uma

Aaron T. Myers
added a comment - 30/Aug/11 19:23 So then I'll go back to my original question - what caused this test to start failing? Perhaps it was previously excluded from even being run, but that was undone by the recent mavenization changes?

Uma Maheswara Rao G
added a comment - 30/Aug/11 19:47 Just now i checked with 22branch with avro 1.3.2 jar. It is working fine.
But in trunk, Avro jars are upgraded to 1.5.2 version. I think because of this up-gradation, this might have happened with mavenization changes.

If we hope to eventually support wire-format interoperability (HADOOP-7347) then we ought to avoid overloading methods in our RPC protocols. If we agree with that, then the fix is to rename one of the overloaded methods, and let Avro serve as a test for these sorts of problems. Another alternative might be to try to fix Avro Reflect to handle this, but I think that could be either tricky, ulgy or both. Or we might insert a non-reflected, manually maintained wrapper layer for each RPC protocol, as has been done with MR2. This is a good practice, but also a lot of work.

Doug Cutting
added a comment - 30/Aug/11 21:54 If we hope to eventually support wire-format interoperability ( HADOOP-7347 ) then we ought to avoid overloading methods in our RPC protocols. If we agree with that, then the fix is to rename one of the overloaded methods, and let Avro serve as a test for these sorts of problems. Another alternative might be to try to fix Avro Reflect to handle this, but I think that could be either tricky, ulgy or both. Or we might insert a non-reflected, manually maintained wrapper layer for each RPC protocol, as has been done with MR2. This is a good practice, but also a lot of work.

Even though Avro RPC is not currently used by Hadoop, checking that RPC methods are not overloaded is probably still useful as it will likely simplify wire compatibilty (HADOOP-7347), whether implemented with Avro or otherwise.

Here's a patch that fixes this test by eliminating overloaded methods in NamenodeProtocols.

There are several cases:

ClientProtocol#delete() had two signatures, one deprecated (HADOOP-4940). This is a private interface and only the non-deprecated version is called any longer, so the deprecated method may now be removed from the protocol.

ClientProtocol#rename() has two signatures, one deprecated (HDFS-654). Both are still used, since they have different semantics. Since the protocol methods are private, it is safe to rename one. The user API can still be overloaded. The new version is now called rename2. The best practice for interoperability when changing semantics of RPC methods is to add a new method.

In two cases, different protocols contained methods with the same signature, with a single implementation in the namenode. Perhaps Avro could be altered to detect when this is the case and not throw an exception, but this is also duplicate code that can be refactored to better reflect the reuse. So this patch adds two new private interfaces BadBlockReportable, shared by ClientProtocol & DatanodeProtocol, and VersionRequestable, shared by DatanodeProtocol & NamenodeProtocol.

Finally, NamenodeProtocol#errorReport() and DatanodeProtocol#errorReport() have different parameters and, as RPC methods, should be named differently if they're to both be implemented by a single service. They are thus renamed namenodeErrorReport() and datanodeErrorReport() respectively.

Doug Cutting
added a comment - 06/Sep/11 19:56 Even though Avro RPC is not currently used by Hadoop, checking that RPC methods are not overloaded is probably still useful as it will likely simplify wire compatibilty ( HADOOP-7347 ), whether implemented with Avro or otherwise.
Here's a patch that fixes this test by eliminating overloaded methods in NamenodeProtocols.
There are several cases:
ClientProtocol#delete() had two signatures, one deprecated ( HADOOP-4940 ). This is a private interface and only the non-deprecated version is called any longer, so the deprecated method may now be removed from the protocol.
ClientProtocol#rename() has two signatures, one deprecated ( HDFS-654 ). Both are still used, since they have different semantics. Since the protocol methods are private, it is safe to rename one. The user API can still be overloaded. The new version is now called rename2. The best practice for interoperability when changing semantics of RPC methods is to add a new method.
In two cases, different protocols contained methods with the same signature, with a single implementation in the namenode. Perhaps Avro could be altered to detect when this is the case and not throw an exception, but this is also duplicate code that can be refactored to better reflect the reuse. So this patch adds two new private interfaces BadBlockReportable, shared by ClientProtocol & DatanodeProtocol, and VersionRequestable, shared by DatanodeProtocol & NamenodeProtocol.
Finally, NamenodeProtocol#errorReport() and DatanodeProtocol#errorReport() have different parameters and, as RPC methods, should be named differently if they're to both be implemented by a single service. They are thus renamed namenodeErrorReport() and datanodeErrorReport() respectively.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

Hadoop QA
added a comment - 06/Sep/11 20:20 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12493204/HDFS-2298.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests:
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1207//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1207//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1207//console
This message is automatically generated.

And, as far as I can tell, Protocol Buffers doesn't either, so Avro is not alone in this.

If we convert to another RPC system we'll probably implement a wrapper layer that converts the overloaded calls to and from the service methods. Even then, this mapping should be as close to 1:1 as possible. Overloading of RPC methods will create confusion.

Doug Cutting
added a comment - 06/Sep/11 20:33 For the record, Thrift doesn't support overloading:
http://wiki.apache.org/thrift/FAQ
And, as far as I can tell, Protocol Buffers doesn't either, so Avro is not alone in this.
If we convert to another RPC system we'll probably implement a wrapper layer that converts the overloaded calls to and from the service methods. Even then, this mapping should be as close to 1:1 as possible. Overloading of RPC methods will create confusion.

Doug Cutting
added a comment - 06/Sep/11 21:35 It turns out that adding new interfaces is a bad idea, as it requires changes to security. So instead I renamed methods for versionRequest() and reportBadBlocks().

Not supporting overloaded methods is fine. BTW with Hadoop-7523 the case of overloaded methods in different protocols will not be an issue since creating a mixin interface will no longer be necessary. I haven't changed the servers to take advantage of this. However its okay for this patch to rename such conflicting methods.

The only snag is that with hadoop-7524 we would have an opportunity to provide compatibility with 20; that will become a little trickier if the method names are changed; however 20 compatibility will be messy anyway..

Yes lets go head with renaming overloaded method. Also lets back port this to 0.23.

Sanjay Radia
added a comment - 14/Sep/11 15:54 Not supporting overloaded methods is fine. BTW with Hadoop-7523 the case of overloaded methods in different protocols will not be an issue since creating a mixin interface will no longer be necessary. I haven't changed the servers to take advantage of this. However its okay for this patch to rename such conflicting methods.
The only snag is that with hadoop-7524 we would have an opportunity to provide compatibility with 20; that will become a little trickier if the method names are changed; however 20 compatibility will be messy anyway..
Yes lets go head with renaming overloaded method. Also lets back port this to 0.23.

Here's a patch that fixes one issue, where someone introduced a new call that needed to be renamed. But, even with this, this patch no longer works. I believe the problem is due to something in HADOOP-7524. The AvroProtocolEngine on the namenode no longer responds using the right protocol when the DataNode contacts it with datanodeVersionRequest. It tries to respond with ClientProtocol rather than DatanodeProtocol. I'll try to look into this more. Sanjay, do you have any idea what's going on here?

Doug Cutting
added a comment - 28/Sep/11 19:37 Here's a patch that fixes one issue, where someone introduced a new call that needed to be renamed. But, even with this, this patch no longer works. I believe the problem is due to something in HADOOP-7524 . The AvroProtocolEngine on the namenode no longer responds using the right protocol when the DataNode contacts it with datanodeVersionRequest. It tries to respond with ClientProtocol rather than DatanodeProtocol. I'll try to look into this more. Sanjay, do you have any idea what's going on here?

Hadoop QA
added a comment - 28/Sep/11 22:25 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12496926/HDFS-2298.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 6 new or modified tests.
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1313//console
This message is automatically generated.

Hadoop QA
added a comment - 28/Sep/11 22:33 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12496926/HDFS-2298.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 6 new or modified tests.
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1314//console
This message is automatically generated.

ClientProtocol#rename() has two signatures, one deprecated (HDFS-654). Both are still used, since they have different semantics. Since the protocol methods are private, it is safe to rename one. The user API can still be overloaded. The new version is now called rename2. The best practice for interoperability when changing semantics of RPC methods is to add a new method.

Suresh Srinivas
added a comment - 28/Sep/11 23:39 - edited ClientProtocol#rename() has two signatures, one deprecated ( HDFS-654 ). Both are still used, since they have different semantics. Since the protocol methods are private, it is safe to rename one. The user API can still be overloaded. The new version is now called rename2. The best practice for interoperability when changing semantics of RPC methods is to add a new method.
Doug you argued against this. See https://issues.apache.org/jira/browse/HADOOP-6240?focusedCommentId=12752647&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12752647
That said, I am okay with renaming the method.
Given that this has been open for a while, for now can you turn off the test?

I still don't like version numbers in methods, but, if semantics are changed in a way that cannot be handled otherwise and there's not an obvious better name, I don't see an alternative. Do you?

Nice of you to look for inconsistencies in what I've said, though. Thanks!

> Given that this has been open for a while, for now can you turn off the test?

I'd prefer to either fix it or remove the code than just disable the test. If there's no will to use Avro RPC in Hadoop then better it should just be removed, no? Sanjay's actively pursuing version interoperability through different means, so maybe it's time to remove this stuff.

This has been patch-available, +1'd and passing tests for a few weeks. Someone should have committed it. Someone like me, I guess, but I was reluctant since I've not been that active here lately. Tests were passing, but have since been broken by HADOOP-7524. I worked today to address that, but if someone instead promotes a patch to remove Avro RPC altogether then I'll stop trying to fix this.

So, no, I do not intend to turn off the test. But if you'd like to 'fix' this that way, go ahead, I won't block you.

Doug Cutting
added a comment - 29/Sep/11 00:11 > Doug you argued against this.
I still don't like version numbers in methods, but, if semantics are changed in a way that cannot be handled otherwise and there's not an obvious better name, I don't see an alternative. Do you?
Nice of you to look for inconsistencies in what I've said, though. Thanks!
> Given that this has been open for a while, for now can you turn off the test?
I'd prefer to either fix it or remove the code than just disable the test. If there's no will to use Avro RPC in Hadoop then better it should just be removed, no? Sanjay's actively pursuing version interoperability through different means, so maybe it's time to remove this stuff.
This has been patch-available, +1'd and passing tests for a few weeks. Someone should have committed it. Someone like me, I guess, but I was reluctant since I've not been that active here lately. Tests were passing, but have since been broken by HADOOP-7524 . I worked today to address that, but if someone instead promotes a patch to remove Avro RPC altogether then I'll stop trying to fix this.
So, no, I do not intend to turn off the test. But if you'd like to 'fix' this that way, go ahead, I won't block you.

Suresh Srinivas
added a comment - 29/Sep/11 04:33 Nice of you to look for inconsistencies in what I've said, though. Thanks!
Well that is not productive or the goal. There was a great deal of time spent discussing this and I remember dropping the name rename2.
So, no, I do not intend to turn off the test. But if you'd like to 'fix' this that way, go ahead, I won't block you.
If the patch is ready it would be good to commit it.

Much simpler version since, with HADOOP-7693 and HDFS-2383, AvroRpcEngine now handles multiple protocols that have overlapping methods and only has issues with a single protocol that has multiple methods of the same name (as would protobuf or Thrift). So the only cases that now need to be addressed by this patch are ClientProtocol#rename and #delete.

Doug Cutting
added a comment - 29/Sep/11 18:56 Much simpler version since, with HADOOP-7693 and HDFS-2383 , AvroRpcEngine now handles multiple protocols that have overlapping methods and only has issues with a single protocol that has multiple methods of the same name (as would protobuf or Thrift). So the only cases that now need to be addressed by this patch are ClientProtocol#rename and #delete.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

Hadoop QA
added a comment - 30/Sep/11 02:57 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12497040/HDFS-2298.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1315//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1315//console
This message is automatically generated.

We can merge it to 0.23 once HADOOP-7524 and HADOOP-7693 are also merged there. Or we could commit the older pre-HADOOP-7693 version of this patch to 0.23, but that would make for gratuitous differences in protocol methods between 0.23 and 0.24. So, if HADOOP-7524 and HADOOP-7693 are not merged to 0.23 then I suggest we instead just remove this test from 0.23 and still merge this patch there, to keep protocols the same between the releases. Opinions, anyone?

Doug Cutting
added a comment - 30/Sep/11 18:07 I committed this to trunk.
We can merge it to 0.23 once HADOOP-7524 and HADOOP-7693 are also merged there. Or we could commit the older pre- HADOOP-7693 version of this patch to 0.23, but that would make for gratuitous differences in protocol methods between 0.23 and 0.24. So, if HADOOP-7524 and HADOOP-7693 are not merged to 0.23 then I suggest we instead just remove this test from 0.23 and still merge this patch there, to keep protocols the same between the releases. Opinions, anyone?

Aaron T. Myers
added a comment - 30/Sep/11 19:58 Thanks Doug. I agree it makes sense to merge this patch into 0.23, and since all of the other changes are compatible with respect to the public API, I think it further makes sense to merge all three.

I believe the plan is to merge this to 0.23 after HADOOP-7524 and then HADOOP-7693 have been merged there. If those issues are not merged to 0.23 then the best option would be simply to remove this test, as was done in HDFS-2383. Sanjay said he intended merge HADOOP-7524 to 0.23, which gates this.

Doug Cutting
added a comment - 20/Oct/11 20:05 I believe the plan is to merge this to 0.23 after HADOOP-7524 and then HADOOP-7693 have been merged there. If those issues are not merged to 0.23 then the best option would be simply to remove this test, as was done in HDFS-2383 . Sanjay said he intended merge HADOOP-7524 to 0.23, which gates this.

Suresh Srinivas
added a comment - 27/Oct/11 17:48 Doug, given that this made change to the protocol, I recommend merging this change to 0.23. This would help in compatibility of 0.23 with future releases. Let me know if you disagree.

Suresh, yes, I very much agree that this should be merged to 0.23. However, since the reason is to keep the protocols compatible, then we need to first merge HADOOP-7524 and HADOOP-7693 to 0.23. The version of this patch that works without those would still leave 0.23 and trunk incompatible.

Doug Cutting
added a comment - 27/Oct/11 18:29 Suresh, yes, I very much agree that this should be merged to 0.23. However, since the reason is to keep the protocols compatible, then we need to first merge HADOOP-7524 and HADOOP-7693 to 0.23. The version of this patch that works without those would still leave 0.23 and trunk incompatible.

Nicholas, so you've elected to merge this without HADOOP-7524 and HADOOP-7693? That will synchronize the protocols but won't fix test failures in 0.23. Is that your intent? It would be much better to present such a proposal before committing.

Doug Cutting
added a comment - 27/Oct/11 20:21 Nicholas, so you've elected to merge this without HADOOP-7524 and HADOOP-7693 ? That will synchronize the protocols but won't fix test failures in 0.23. Is that your intent? It would be much better to present such a proposal before committing.

Tsz Wo Nicholas Sze
added a comment - 27/Oct/11 20:28 Hi Doug,
Suresh has suggested to merge this to 0.23 on the hdfs-dev mailing list. Below is his justification.
> HADOOP-2298 TestDFSOverAvroRpc fails
> Justification: This makes a protocol method name change. Required for
> compatibility of 0.23 with future releases.
> Risk: low
Do you agree? Or we may revert the 0.23 commit.

Suresh, yes, I very much agree that this should be merged to 0.23. However, since the reason is to keep the protocols compatible, then we need to first merge HADOOP-7524 and HADOOP-7693 to 0.23. The version of this patch that works without those would still leave 0.23 and trunk incompatible.

HADOOP-7524 made changes in ipc/rpc layer mechanism. I am not sure what you mean by 0.23 and trunk would be incompatible without these.

Suresh Srinivas
added a comment - 27/Oct/11 23:01 Suresh, yes, I very much agree that this should be merged to 0.23. However, since the reason is to keep the protocols compatible, then we need to first merge HADOOP-7524 and HADOOP-7693 to 0.23. The version of this patch that works without those would still leave 0.23 and trunk incompatible.
HADOOP-7524 made changes in ipc/rpc layer mechanism. I am not sure what you mean by 0.23 and trunk would be incompatible without these.

As the above reports from Jenkins show, merging this patch alone will not cause TestDfsOverAvroRpc to stop failing. It does make the protocols compatible, which is good. But if we want this test to pass (the actual point of this issue) then we need to also merge the other two issues I've named above.

Doug Cutting
added a comment - 28/Oct/11 21:21 As the above reports from Jenkins show, merging this patch alone will not cause TestDfsOverAvroRpc to stop failing. It does make the protocols compatible, which is good. But if we want this test to pass (the actual point of this issue) then we need to also merge the other two issues I've named above.