giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sergey Edunov" <edu...@gmail.com>
Subject Re: Review Request 25971: Adds Giraph Debugger (Graft) to Giraph trunk
Date Fri, 17 Oct 2014 17:53:32 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25971/#review57157
-----------------------------------------------------------



giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/Server.java
<https://reviews.apache.org/r/25971/#comment97661>

    Will it be replaced with something real?



giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/Server.java
<https://reviews.apache.org/r/25971/#comment97659>

    Is it some custom tag library? Maybe you want annotation instead? I'm not sure what it
means



giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/ServerHttpHandler.java
<https://reviews.apache.org/r/25971/#comment97641>

    I'm not sure about our policy on using oracle specific features. How hard it would be
to switch to jetty? We already have it in classpath anyway.



giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/ServerHttpHandler.java
<https://reviews.apache.org/r/25971/#comment97652>

    Why would you need it?



giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/ServerHttpHandler.java
<https://reviews.apache.org/r/25971/#comment97654>

    Can you please use standard javadoc tags?



giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/ServerHttpHandler.java
<https://reviews.apache.org/r/25971/#comment97658>

    NumberFormatException extends IllegalArgumentException so this condition is always false



giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/AbstractInterceptingComputation.java
<https://reviews.apache.org/r/25971/#comment97633>

    These fields are not constants, so should not be named with uppercase letters. And I'm
not sure why they made static too. Is it necessary?



giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/InstrumentGiraphClasses.java
<https://reviews.apache.org/r/25971/#comment97684>

    Just createTempDirectory(TMP_DIR_NAME_PREFIX) will work



giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/InstrumentGiraphClasses.java
<https://reviews.apache.org/r/25971/#comment97681>

    Do e.printStackTrace() at least. We should know which classes were not found.



giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/InstrumentGiraphClasses.java
<https://reviews.apache.org/r/25971/#comment97686>

    Either mc or masterComputeClass is redundand



giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/InstrumentGiraphClasses.java
<https://reviews.apache.org/r/25971/#comment97679>

    targetTopClass is always CtClass, how can this equals be satisfied? Did you mean 
    targetTopClass.toClass().equals(Object.class) ?



giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/BaseWrapper.java
<https://reviews.apache.org/r/25971/#comment97663>

    just 
    method.invoke(urlClassLoader, u); 
    will work fine


This diff is huge and takes a lot of time to comprehend, I focussed on code quality issues
now and will need another pass for more general issues.

- Sergey Edunov


On Sept. 24, 2014, 1:07 a.m., Jaeho Shin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25971/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2014, 1:07 a.m.)
> 
> 
> Review request for giraph and Sergey Edunov.
> 
> 
> Bugs: GIRAPH-905
>     https://issues.apache.org/jira/browse/GIRAPH-905
> 
> 
> Repository: giraph-git
> 
> 
> Description
> -------
> 
> A rather large patch to include Giraph Debugger (Graft) in Giraph trunk.  Based on [222e5e
of semihsalihoglu/graft](https://github.com/semihsalihoglu/graft/commit/222e5e7c86c3c4122c1b88336b17966d4e7728d0).
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 552cca9 
>   giraph-debugger/.gitignore PRE-CREATION 
>   giraph-debugger/README.md PRE-CREATION 
>   giraph-debugger/giraph-debug PRE-CREATION 
>   giraph-debugger/gui PRE-CREATION 
>   giraph-debugger/pom.xml PRE-CREATION 
>   giraph-debugger/src/main/assembly/compile.xml PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/CommandLine.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/DebugConfig.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/exceptiondebug/BuggySimpleTriangleClosingComputation.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/exceptiondebug/SimpleTriangleClosingDebugConfig.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/exceptiondebug/package-info.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/instrumented/BuggyConnectedComponentsDebugComputationModified.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/instrumented/BuggyConnectedComponentsDebugComputationToRun.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/instrumented/BuggySimpleShortestPathsDebugComputationModified.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/instrumented/BuggySimpleShortestPathsDebugComputationToRun.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/instrumented/BuggySimpleTriangleClosingDebugComputationModified.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/instrumented/BuggySimpleTriangleClosingDebugComputationToRun.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/instrumented/package-info.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/integrity/BuggyConnectedComponentsComputation.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/integrity/ConnectedComponentsMsgIntegrityDebugConfig.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/integrity/ConnectedComponentsVValueIntegrityDebugConfig.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/integrity/package-info.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/simpledebug/BuggySimpleShortestPathsComputation.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/simpledebug/SimpleShortestPathsDebugConfig.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/simpledebug/SimpleShortestPathsMaster.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/examples/simpledebug/package-info.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/Server.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/ServerHttpHandler.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/ServerUtils.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/gui/package-info.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/AbstractInterceptingComputation.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/AbstractInterceptingMasterCompute.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/BottomInterceptingComputation.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/BottomInterceptingMasterCompute.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/CommonVertexMasterInterceptionUtil.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/InstrumentGiraphClasses.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/Intercept.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/UserComputation.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/UserMasterCompute.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/instrumenter/package-info.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/mock/ComputationComputeTestGenerator.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/mock/FormatHelper.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/mock/MasterComputeTestGenerator.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/mock/PrefixedClasspathResourceLoader.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/mock/TestGenerator.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/mock/TestGraphGenerator.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/mock/VelocityBasedGenerator.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/mock/package-info.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/package-info.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/AggregatedValueWrapper.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/AggregatorWrapper.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/BaseScenarioAndIntegrityWrapper.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/BaseWrapper.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/CommonVertexMasterContextWrapper.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/DebuggerUtils.java PRE-CREATION

>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/ExceptionWrapper.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/GiraphMasterScenarioWrapper.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/GiraphVertexScenarioWrapper.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/MsgIntegrityViolationWrapper.java
PRE-CREATION 
>   giraph-debugger/src/main/java/org/apache/giraph/debugger/utils/package-info.java PRE-CREATION

>   giraph-debugger/src/main/protobuf/giraph_aggregator.proto PRE-CREATION 
>   giraph-debugger/src/main/protobuf/integrity.proto PRE-CREATION 
>   giraph-debugger/src/main/protobuf/scenario.proto PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/css/app.css PRE-CREATION

>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/css/slider/slider.css
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/css/valpanel.css
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/img/details_close.png
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/img/details_open.png
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/img/preloader.gif
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/index.html PRE-CREATION

>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/js/debugger.js PRE-CREATION

>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/js/editor.core.js
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/js/editor.utils.js
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/js/slider/bootstrap-slider.js
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/js/utils.js PRE-CREATION

>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/js/utils.sampleGraphs.js
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/gui/js/valpanel.js PRE-CREATION

>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/mock/ComputeSetUpFuncTemplate.vm
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/mock/ComputeTestFuncTemplate.vm
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/mock/ComputeTestTemplate.vm
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/mock/MasterComputeTestTemplate.vm
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/mock/ReadWritableFromByteArrayTemplate.vm
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/mock/ReadWritableFromStringTemplate.vm
PRE-CREATION 
>   giraph-debugger/src/main/resources/org/apache/giraph/debugger/mock/TestGraphTemplate.vm
PRE-CREATION 
>   giraph-debugger/src/test/java/org/apache/giraph/debugger/instrumenter/test/basecompute/BaseComputation.java
PRE-CREATION 
>   giraph-debugger/src/test/java/org/apache/giraph/debugger/instrumenter/test/basecompute/CommonDebugConfig.java
PRE-CREATION 
>   giraph-debugger/src/test/java/org/apache/giraph/debugger/instrumenter/test/basecompute/DerivedComputation.java
PRE-CREATION 
>   giraph-debugger/src/test/java/org/apache/giraph/debugger/instrumenter/test/basecompute/package-info.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25971/diff/
> 
> 
> Testing
> -------
> 
> Manually followed the steps for debugging shown in the [giraph-debugger/README.md](https://github.com/semihsalihoglu/graft#launching-giraph-jobs-with-graft),
and confirmed generated test codes for Computation.compute() and MasterCompute with GUI and
CLI reproduces the context.
> 
> For launching a buggy example (included under giraph-debugger), used the following command:
> ```
> ../giraph-debugger/giraph-debug     ../giraph-debugger/target/giraph-debugger-1.1.0-SNAPSHOT-for-hadoop-1.2.1-jar-with-dependencies.jar
org.apache.giraph.GiraphRunner     org.apache.giraph.debugger.examples.simpledebug.BuggySimpleShortestPathsComputation
-mc org.apache.giraph.debugger.examples.simpledebug.SimpleShortestPathsMaster     -vif org.apache.giraph.io.formats.JsonLongDoubleFloatDoubleVertexInputFormat
    -vip shortestPathsInputGraph     -vof org.apache.giraph.io.formats.IdWithValueTextOutputFormat
    -op shortestPathsOutputGraph.$RANDOM     -w 1     -ca giraph.SplitMasterWorker=false 
   #
> ```
> 
> 
> Thanks,
> 
> Jaeho Shin
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message