[
https://issues.apache.org/jira/browse/OOZIE-1200?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13574146#comment-13574146
]
Alejandro Abdelnur commented on OOZIE-1200:
-------------------------------------------
Second round of comments based on todays commit to the hcat-intre branch:
{code}
1* CoordActionInputCheckXCommand, execute() calls updateCoordAction in finally
BLOCK, if there is an exception halfway the execution this will save an
inconsistent state, this update should be done at the end of the try BLOCK.
2* CoordActionInputCheckXCommand, pathExists() can extract the user from the
conf within itself instead doing that outside and passing it as parameter.
3* CoordActionUpdatePushMissingDependency, execute() has 3 returns, it should
have one at the end of the method. (#)
4* CoordActionUpdatePushMissingDependency, execute(), the loop that populates
missingDepsAfterCheck list can be replace with a simple
missingDepsAfterCheck.removeAll(availDepList). (#)
5* CoordActionUpdatePushMissingDependency, unregisterAvailableDependencies(),
first log line has typo. (#)
6* CoordCommandUtils has false formatting changes lines 209, 231, 450
7* CoordELEvaluator, getDSObject(), the changes, moving code around, are not
needed.
8* CoordELFunctions, coord_futureRange_sync(), the uriContext should be
destroyed in a finally BLOCK to ensure it is destroyed when an exception
halfway happens.
9* CoordELFunctions, coord_latestRange_sync(), the uriContext should be
destroyed in a finally BLOCK to ensure it is destroyed when an exception
halfway happens.
10* CoordinatorFunctionalSpec, section #5.2, the example should include some
template vars (${YEAR}, ${MONTH}, etc)
11* CoordinatorFunctionalSpec, section #6.8.3, shouldn't map-reduce be
mentioned as a type too?
12* CoordinatorFunctionalSpec, there are 2 sections #6.8.4
13* CoordinatorFunctionalSpec, first section #6.8.4, 'Ensure the following JARs
are in ...' this does not belong to the spec, this should be an an
HCatIntegrationGuide page, and as discussed before, the hcat JARs should be
avail in the sharelibs. (#)
14* CoordinatorFunctionalSpec, there is no section #6.8.6
15* CoordinatorFunctionalSpec, second section #6.8.7, example two, region gets
resolved into ${region} or into the value of the job region job property? this
is not clear
16* CoordPushDependencyCheckXCommand, loadState() and verifyPrecondition()
should repeat the load * verification done by the eager counterparts as this is
done within a lock (#)
17* HadoopAccessorService, it seems at least a commit from trunk is missing
here, please synch up with the latest trunk
18* HCatAccessorService, we need to document the mapping rules, this should be
done in the new doc mentioned in comment #13 (#)
19* HCatAccessorService, the key creation should be done in a private method,
currently its construction is repeated in 4 places (#)
20* HCatLauncherURIHandler, getHCatClient(), the Ssytem print out shoud print
the current user not the login user.
21* HCatURIHandler/FSURIHandler/URIHandler, why 2 signatures of the exists()?
the one with the context should be enough
22* HCatURIHandler, the exits() signature that takes a context should not close
the context, the caller is responsible for that, so it can reuse the context
23* JMSAccessorService, registerForNotification() it has multiple returns, it
should exit at the end of the method
24* JMSAccessorService, the unregister must synchronize on topicsMap before
removing stuff
25* JMSExceptionListener, javadoc mentions wrong parameter name
26* LauncherMapper seems to miss a commit from trunk, please verify
27* MappingRule, missing License header
28* OozieCLI has false changes or a missing commit from trunk (#)
29* PigActionExecutor seems to have a missing commit from trunk
30* core/pom.xml, do we need activemq-all as compile dependency? isn't there a
client JAR?
31* hadoop-2 poms seem to miss a commit from trunk
32* URIHandler, why registerForNotification() does not take a URIContext? (#)
33* URIHandlerService, init() it should trim the classname before attempting to
do a Class.forName()
34* WaitingAction, why is serializable? (#)
{code}
The comments ending with (#) are dup from the first review, they have not been
addressed yet.
> Review of HCAT integration branch
> ---------------------------------
>
> Key: OOZIE-1200
> URL: https://issues.apache.org/jira/browse/OOZIE-1200
> Project: Oozie
> Issue Type: Sub-task
> Components: coordinator, docs
> Reporter: Alejandro Abdelnur
> Fix For: trunk
>
> Attachments: HCat-commandcoord-package-review.patch,
> HCat-depedency-cache-review.patch, HCat-dependency-package-review.patch,
> Hcat-service-util-tools-action-jms-packages-review-1.patch,
> Hcat-service-util-tools-action-jms-packages-review.patch,
> HCatSpec-comments.patch
>
>
> Given the number of commits (and fixes) that when into this branch, I think
> the best way to review it is by posting patches with comments through out the
> code. Then we can follow up the discussion as comments in this JIRA.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
