----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12543/#review23351 -----------------------------------------------------------
When reviewing such a big diff, it would help to have a description of the main classes introduced and their role. Anyway, the overall approach looks good. The circular reference used to wrap JythonComputation is clever, but kind of nasty. I wish it could be avoided. The PerGraphType- classes seem a little overengineered, especially since they're only used once. I wonder if it's feasible to move some of that logic in place (in ICGConfiguration). Also, you sneaked in some seemingly unrelated changes in metrics. Can you leave those for an appropriate JIRA? giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java <https://reviews.apache.org/r/12543/#comment47237> giraphParametersConfiguration? giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java <https://reviews.apache.org/r/12543/#comment47241> Can you expand here on why this is needed? giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java <https://reviews.apache.org/r/12543/#comment47238> updateConfiguration doesn't convey what the method does. Maybe addGiraphParameters? giraph-core/src/main/java/org/apache/giraph/factories/ValueFactory.java <https://reviews.apache.org/r/12543/#comment47239> If you made newInstance() common, then why not go all the way and also put getValueClass() directly here? giraph-core/src/main/java/org/apache/giraph/graph/AbstractComputation.java <https://reviews.apache.org/r/12543/#comment47254> The Javadoc here is basically the same as for the Computation interface. We should explain the purpose of this abstract implementation (i.e., this is the standard class that one should extend to write algorithms in pure Java). Also, it's a bit unfortunate that now users have to extend AbstractComputation instead of Computation, from a naming point of view. I would prefer this to still be Computation, but then I'm not sure what to call the interface. giraph-core/src/main/java/org/apache/giraph/graph/Computation.java <https://reviews.apache.org/r/12543/#comment47242> Revise the comment, this is not a class anymore. giraph-core/src/main/java/org/apache/giraph/graph/GraphTaskManager.java <https://reviews.apache.org/r/12543/#comment47243> What does this deletion have to do with the diff? giraph-core/src/main/java/org/apache/giraph/job/HadoopUtils.java <https://reviews.apache.org/r/12543/#comment47244> What's with the removal of these task/job ids? I don't have enough context to understand (pun intended). giraph-core/src/main/java/org/apache/giraph/job/HadoopUtils.java <https://reviews.apache.org/r/12543/#comment47245> makeTaskContext() calls makeTaskAttemptContext() with an empty configuration? What is the lack of 'Attempt' in the name supposed to convey? giraph-core/src/main/java/org/apache/giraph/jython/JythonGiraphCompute.java <https://reviews.apache.org/r/12543/#comment47246> Why Compute and not Computation? giraph-core/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java <https://reviews.apache.org/r/12543/#comment47247> Unrelated to the issue. giraph-core/src/main/java/org/apache/giraph/metrics/SuperstepMetricsRegistry.java <https://reviews.apache.org/r/12543/#comment47248> Unrelated to the issue. giraph-core/src/main/java/org/apache/giraph/utils/ConfigurationUtils.java <https://reviews.apache.org/r/12543/#comment47250> There's an extra space here. giraph-core/src/main/java/org/apache/giraph/utils/ConfigurationUtils.java <https://reviews.apache.org/r/12543/#comment47249> Is this meant to be used in Jython code, where typing is dynamic? If so, better make that clear in the comments. You might even want to isolate functionality like this in Jython-specific utils, if we don't expect (and I hope we don't) people to call this in Java code and incur in an unchecked exception. giraph-core/src/main/java/org/apache/giraph/utils/ConfigurationUtils.java <https://reviews.apache.org/r/12543/#comment47252> All this method does is check the argument for null, and call another method. I don't see the big value of it. I would check for null as early as possible. giraph-hive/src/test/resources/org/apache/giraph/jython/fake-label-propagation-worker.py <https://reviews.apache.org/r/12543/#comment47253> Weren't we getting rid of explicit calls to wrap()? - Alessandro Presta On July 17, 2013, 4:22 p.m., Nitay Joffe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12543/ > ----------------------------------------------------------- > > (Updated July 17, 2013, 4:22 p.m.) > > > Review request for giraph. > > > Bugs: GIRAPH-717 > https://issues.apache.org/jira/browse/GIRAPH-717 > > > Repository: giraph-git > > > Description > ------- > > See JIRA > > > Diffs > ----- > > CHANGELOG 8cc0945b563b75f820c1336e7098e9581c27f93d > findbugs-exclude.xml 2f7f4007cbfb45f077377328bd43b09ba0424af4 > giraph-core/pom.xml cab0157803d130ab5ab342c6c5c6d5744f8bdaa3 > > giraph-core/src/main/java/org/apache/giraph/comm/messages/ByteArrayMessagesPerVertexStore.java > ba6d020726f3609c7cc3a9469e76058b787c576f > > giraph-core/src/main/java/org/apache/giraph/comm/messages/MessagesIterable.java > f3f327ffdfe4ec68cb5a61a3bd16f0cf82bd7df9 > > giraph-core/src/main/java/org/apache/giraph/comm/messages/OneMessagePerVertexStore.java > bedaf4818adaf26cbd5b88588efe916bc00daa10 > > giraph-core/src/main/java/org/apache/giraph/comm/messages/out_of_core/SequentialFileMessageStore.java > 1ff0e189cd954c98c594288a9b9ea98bee773960 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java > 66558342df172aa85e75ad4aaa1c8a93568843eb > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java > 74f1ba586fd3c84200b263987315081eb6d47caa > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java > c4cc96f82d8ea8180009f8d082366d64d2b28b0b > > giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java > db24423cdefb968f71fa2174a1b9788c945e6b24 > giraph-core/src/main/java/org/apache/giraph/conf/PerGraphTypeBoolean.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/conf/PerGraphTypeBooleanConfOption.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/conf/PerGraphTypeEnum.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/conf/PerGraphTypeEnumConfOption.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/factories/AbstractMessageValueFactory.java > f426d5f55e5ce3eee98bb8c3638ec9e253c1a26f > > giraph-core/src/main/java/org/apache/giraph/factories/DefaultEdgeValueFactory.java > 55d27e781290c406011a2ae201a811f4d85e8f31 > > giraph-core/src/main/java/org/apache/giraph/factories/DefaultVertexIdFactory.java > 4977b44cc5fdc6ca27d789cf66b542ba177d12af > > giraph-core/src/main/java/org/apache/giraph/factories/DefaultVertexValueFactory.java > caf312f6f5507281ead0b9f94efacab124ac239c > giraph-core/src/main/java/org/apache/giraph/factories/EdgeValueFactory.java > 751c6e61d653519a8f7438b1dfc103b86774705f > > giraph-core/src/main/java/org/apache/giraph/factories/MessageValueFactory.java > a95e1a27a1235cbd57ad48fc77ca1d183bb96075 > > giraph-core/src/main/java/org/apache/giraph/factories/TestMessageValueFactory.java > 3d376de6272f0c39702c2e174e6fc66f549721e0 > giraph-core/src/main/java/org/apache/giraph/factories/ValueFactory.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/factories/ValueFactoryBase.java > f92fb747b0c1f6d8b3a87a9e2813c3f0bcae4ae8 > giraph-core/src/main/java/org/apache/giraph/factories/VertexIdFactory.java > 328bda8a94a8b8f6aad43dabaadea263cc503650 > > giraph-core/src/main/java/org/apache/giraph/factories/VertexValueFactory.java > d2f62ab99d6cf2d26b1a2fa829e2213ff4c7da5d > giraph-core/src/main/java/org/apache/giraph/graph/AbstractComputation.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/graph/BasicComputation.java > 180c5d34a7c02c37ff935941b02d50dd3018eff3 > giraph-core/src/main/java/org/apache/giraph/graph/Computation.java > 87d587928aa56c3bbabacaf2f9c8dfd5634d79fa > giraph-core/src/main/java/org/apache/giraph/graph/GraphTaskManager.java > b0982b33fff73409a328cec0861b0a4f72ec6bad > giraph-core/src/main/java/org/apache/giraph/graph/GraphType.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedEdgeInputFormat.java > c3adf4c39b1ac4847e1c21af8a3e48064e41219e > > giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedEdgeReader.java > e3b3689dde83ae8a80f67f71dc2fd912b93f7ac0 > > giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedVertexInputFormat.java > a58a32dd101fd63b02256238cfaefcec16d142f0 > > giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedVertexOutputFormat.java > bffa33036d19d01a6153888b82264f6e80e4502f > > giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedVertexReader.java > bf0a212efa0826a923583dd15b75e673ef0d19b0 > giraph-core/src/main/java/org/apache/giraph/job/HadoopUtils.java > d5095bcaeb0c964cc071ce002af706ae0607e686 > giraph-core/src/main/java/org/apache/giraph/jython/JythonComputation.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/JythonComputationFactory.java > 80e4e7620ff6953b167ef4ff81177fb9914a8893 > giraph-core/src/main/java/org/apache/giraph/jython/JythonGiraphCompute.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/jython/JythonJob.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/jython/JythonOptions.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/jython/JythonUtils.java > e747e8475281b55acd75273a7b5b9ef1a256ef14 > > giraph-core/src/main/java/org/apache/giraph/jython/factories/JythonComputationFactory.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/factories/JythonEdgeValueFactory.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/factories/JythonFactoryBase.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/factories/JythonIncomingMessageValueFactory.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/factories/JythonMessageValueFactory.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/factories/JythonOutgoingMessageValueFactory.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/factories/JythonVertexIdFactory.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/factories/JythonVertexValueFactory.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/factories/package-info.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/wrappers/JythonWrapperBase.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/wrappers/JythonWritableWrapper.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/jython/wrappers/package-info.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java > a3427dc3a8137357e3bb24a4a48398e8a64643b6 > giraph-core/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java > 7d980ea69ae22fba89e336b261d24afec651e585 > > giraph-core/src/main/java/org/apache/giraph/metrics/SuperstepMetricsRegistry.java > c58a486064c23a7478da15b81c45f1bdf590d71c > > giraph-core/src/main/java/org/apache/giraph/types/BooleanToBooleanWritableWrapper.java > d0637504ce4d5ec001503d571cb1dd2b7d49e606 > > giraph-core/src/main/java/org/apache/giraph/types/ByteToByteWritableWrapper.java > 1b36178080fd69753e0b8c2e9f6a942e36e04014 > > giraph-core/src/main/java/org/apache/giraph/types/ByteToIntWritableWrapper.java > adbbb98947b320ba4db97fe849f2a6bff3320898 > > giraph-core/src/main/java/org/apache/giraph/types/ByteToLongWritableWrapper.java > a4f22a5d523858b5a8e63987bb43e448c768d7e4 > > giraph-core/src/main/java/org/apache/giraph/types/DoubleToDoubleWritableWrapper.java > a1fae98d193ec8a77787ee587457a59dca121e72 > > giraph-core/src/main/java/org/apache/giraph/types/FloatToDoubleWritableWrapper.java > d51edcab96fa927d61bdd7a8cfb6c92e99e4bf20 > > giraph-core/src/main/java/org/apache/giraph/types/FloatToFloatWritableWrapper.java > 4f4dd9796e308474cfbba39dcff7c0a718afbfd4 > > giraph-core/src/main/java/org/apache/giraph/types/IntToIntWritableWrapper.java > 4d868ac4cead60c19a3a1ee8ec0c7d1e43da736e > > giraph-core/src/main/java/org/apache/giraph/types/IntToLongWritableWrapper.java > 9340d5222633c5ed57c25ef767fdb06a5735f208 > > giraph-core/src/main/java/org/apache/giraph/types/LongToLongWritableWrapper.java > 2211ccb926d71319b7fe5825c4b9fd590a1b7286 > > giraph-core/src/main/java/org/apache/giraph/types/ShortToIntWritableWrapper.java > 6df538ade7ceaad55fbb4a47f9eaf39c58432793 > > giraph-core/src/main/java/org/apache/giraph/types/ShortToLongWritableWrapper.java > f3d56b5c77df25619ac21d52d74b3badbac72d86 > giraph-core/src/main/java/org/apache/giraph/types/WritableWrapper.java > edb62a5b91d6396f968c28fb3bf297ec46e44af4 > > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayVertexIdMessages.java > 3c8afd025c85dd97e90831ea0de2837dfa406b31 > giraph-core/src/main/java/org/apache/giraph/utils/ConfigurationUtils.java > 0424ff541f9ee12feac0e370d71085f8d91b9019 > giraph-core/src/main/java/org/apache/giraph/utils/ReflectionUtils.java > 908376993c45c72bd531eabf6d5817d1aece197a > giraph-core/src/test/java/org/apache/giraph/io/TestEdgeInput.java > 425fcba10ac8cd63e93520d6da0f064155f29d5e > giraph-core/src/test/java/org/apache/giraph/jython/TestJython.java > b5110bb5996235bfb892c578bdaeb38d7bc6f2b5 > giraph-core/src/test/java/org/apache/giraph/jython/TestJythonBasic.java > PRE-CREATION > > giraph-core/src/test/java/org/apache/giraph/jython/TestJythonComputation.java > PRE-CREATION > > giraph-core/src/test/java/org/apache/giraph/jython/TestJythonWritableWrapper.java > PRE-CREATION > > giraph-core/src/test/java/org/apache/giraph/master/TestComputationCombinerTypes.java > f9bd4e42f5bfa479cad38bec820e8b21b633bacd > giraph-core/src/test/java/org/apache/giraph/master/TestSwitchClasses.java > 4069972c747fc3be86d99f15aef21f99c49ad940 > giraph-core/src/test/java/org/apache/giraph/utils/TestReflectionUtils.java > 1c3eed3d46ccc9dbcd5ed100146a16216641677c > giraph-examples/src/test/java/org/apache/giraph/TestBspBasic.java > f9e77affacbb4890e2006383c37e3bb281cda0d7 > > giraph-examples/src/test/java/org/apache/giraph/vertex/TestComputationTypes.java > fe354fadfe29b71bbd44158939116053e54683d5 > > giraph-hcatalog/src/main/java/org/apache/giraph/io/hcatalog/HCatGiraphRunner.java > 6135c7d1c4e816f2dbbf433910552219a26a5f32 > > giraph-hive/src/main/java/org/apache/giraph/hive/column/HiveReadableColumn.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/column/HiveWritableColumn.java > PRE-CREATION > giraph-hive/src/main/java/org/apache/giraph/hive/column/package-info.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/common/GiraphHiveConstants.java > e492694959d8cb488b4bc9d5a1bf4b321fe396f2 > giraph-hive/src/main/java/org/apache/giraph/hive/common/HiveUtils.java > ae2a402c69da105d7127ed2396c0d8e661d21be8 > > giraph-hive/src/main/java/org/apache/giraph/hive/common/LanguageAndType.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/input/edge/AbstractHiveToEdge.java > a3c323e3d8d964295ebfa0de6ca4fbbe52b9bbe2 > > giraph-hive/src/main/java/org/apache/giraph/hive/input/edge/TypedHiveToEdge.java > ac62827efb5a8e0cc12c144ba2c5a31eb7018e40 > > giraph-hive/src/main/java/org/apache/giraph/hive/input/vertex/AbstractHiveToVertex.java > 45cc20978ff43b5bde5fca64f5c2718d27ecf5fc > > giraph-hive/src/main/java/org/apache/giraph/hive/input/vertex/TypedHiveToVertex.java > 5ca7d34c49ba44b7626df900835e6d925c4480e6 > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/HiveJythonRunner.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/HiveJythonUtils.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonColumnReader.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonColumnWriter.java > PRE-CREATION > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonHiveIO.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonHiveReader.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonHiveToEdge.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonHiveToVertex.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonHiveWriter.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonReadableColumn.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/JythonVertexToHive.java > PRE-CREATION > giraph-hive/src/main/java/org/apache/giraph/hive/jython/package-info.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/output/TypedVertexToHive.java > 8d6fd9a4ca589291aae7c994955fe4bc845b199f > > giraph-hive/src/main/java/org/apache/giraph/hive/primitives/PrimitiveValueReader.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/primitives/PrimitiveValueWriter.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/primitives/package-info.java > PRE-CREATION > giraph-hive/src/main/java/org/apache/giraph/hive/types/HiveValueReader.java > c0fad122f302a3cef4ffec8ad51ff116c8a8f51c > giraph-hive/src/main/java/org/apache/giraph/hive/types/HiveValueWriter.java > f38f79a482c627cdd2f6797db684d83e2717b724 > > giraph-hive/src/main/java/org/apache/giraph/hive/types/HiveVertexIdReader.java > 1ad6f7f734bd34b424ee0f1d9536ee4e5c3579d5 > > giraph-hive/src/main/java/org/apache/giraph/hive/types/HiveVertexIdWriter.java > 33196f5bbfc9c10fc944ad14f14dbdb7c1f91fdd > > giraph-hive/src/main/java/org/apache/giraph/hive/types/TypedValueReader.java > e9b068732459f0d586187b7a86782859d44590ac > > giraph-hive/src/main/java/org/apache/giraph/hive/types/TypedValueWriter.java > 8841bda5e9400859776acd6df6b04c3fc4b97b13 > > giraph-hive/src/main/java/org/apache/giraph/hive/types/TypedVertexIdReader.java > 6d80d37082ffe929a5e00c3ae17ef57e200659b9 > > giraph-hive/src/main/java/org/apache/giraph/hive/types/TypedVertexIdWriter.java > aece54b5715eb29cd220772dc5061266501eed11 > giraph-hive/src/main/java/org/apache/giraph/hive/types/package-info.java > 700463766f118e4a18ec8a8d2a9156adb16b2b31 > > giraph-hive/src/main/java/org/apache/giraph/hive/values/HiveValueReader.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/values/HiveValueWriter.java > PRE-CREATION > giraph-hive/src/main/java/org/apache/giraph/hive/values/package-info.java > PRE-CREATION > giraph-hive/src/test/java/org/apache/giraph/hive/GiraphHiveTestBase.java > 49a36d094869463ec4b90abc5e787ca5a4b80121 > giraph-hive/src/test/java/org/apache/giraph/hive/Helpers.java > 00c00ca5ecacb94517e4e7bf77d0f25630733a29 > > giraph-hive/src/test/java/org/apache/giraph/hive/input/HiveEdgeInputTest.java > d5bbb954d096d15b720ff0b2590b75de67d776a2 > > giraph-hive/src/test/java/org/apache/giraph/hive/input/HiveVertexInputTest.java > af850d5c028fe9528733a3054c217fa690712684 > > giraph-hive/src/test/java/org/apache/giraph/hive/jython/TestHiveJythonComplexTypes.java > PRE-CREATION > > giraph-hive/src/test/java/org/apache/giraph/hive/jython/TestHiveJythonPrimitives.java > PRE-CREATION > giraph-hive/src/test/java/org/apache/giraph/hive/output/HiveOutputTest.java > 4d4d976dd99ca52d4a9bd9e3fef4c29119143721 > > giraph-hive/src/test/resources/org/apache/giraph/jython/count-edges-launcher.py > PRE-CREATION > > giraph-hive/src/test/resources/org/apache/giraph/jython/fake-label-propagation-launcher.py > PRE-CREATION > > giraph-hive/src/test/resources/org/apache/giraph/jython/fake-label-propagation-worker.py > PRE-CREATION > pom.xml 7a79aede23118c1d0d33c594bcf3f25319027aa0 > > Diff: https://reviews.apache.org/r/12543/diff/ > > > Testing > ------- > > > Thanks, > > Nitay Joffe > >
