> On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > checkstyle/src/main/resources/falcon/checkstyle.xml, line 220 > > <https://reviews.apache.org/r/17580/diff/1/?file=457487#file457487line220> > > > > Why is this downgraded to warning ?
Since there are TODOs in the code and the build fails. Its quite annoying since I cannot fix the todos at this time. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 556 > > <https://reviews.apache.org/r/17580/diff/1/?file=457489#file457489line556> > > > > Remove ? I had removed such methods earlier but I was asked to add back. Not sure if these will be used in future. :-) > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/ClusterHelper.java, line 45 > > <https://reviews.apache.org/r/17580/diff/1/?file=457496#file457496line45> > > > > Why is fs.default.name set twice through two different key constants? one is fs.default.name and the other is fs.defaultFS for 2.0. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/AbstractCatalogService.java, > > line 41 > > <https://reviews.apache.org/r/17580/diff/1/?file=457491#file457491line41> > > > > Why is the metaStorePrincipal only in few methods ? Looks like they are > > the initial set of calls into the service and would end up caching the > > client object. But that is not a good assumption to make and can possibly > > introduce errors. Would prefer that either metaStorePrinicipal is in every > > call or allow something similar doAs functionality. Only these 2 methods are invoked from with in falcon server but everything else is with in oozie namespace which oozie handles. May be I can add a comment. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/HiveCatalogService.java, > > line 94 > > <https://reviews.apache.org/r/17580/diff/1/?file=457492#file457492line94> > > > > This seems to simply login as the current user, while the > > metaStorePrincipal is set as a var. Is this adequate to proxy for hcat > > client ? Yes and answer is 2 fold: * hive does not need authorization for metadata operations * hive does not support proxy doAs > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/parser/ClusterEntityParser.java, > > line 119 > > <https://reviews.apache.org/r/17580/diff/1/?file=457499#file457499line119> > > > > Does it make sense to skip the validation for hftp then ? This is fixed internally and will be committed once its tested. In the meanwhile, falcon should encourage folks to use webhdfs since that works and hftp will be deprecated. Also, webhdfs has been tested extensively with distcp. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java, > > line 101 > > <https://reviews.apache.org/r/17580/diff/1/?file=457502#file457502line101> > > > > This would set the umask for all files created henceforth through that > > file system object (which by the way is cached). There may be unwanted side > > effects with this. Can we simply set permission on the storePath dir > > instead ? Thats a good point, I tried with FsPermissions instead but did not work. Its funny that I have run into issues a few time. But will change. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/util/Preconditions.java, line 1 > > <https://reviews.apache.org/r/17580/diff/1/?file=457510#file457510line1> > > > > Guava ? Thought about it but just for one class? > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > messaging/src/test/java/org/apache/falcon/messaging/FalconTopicProducerTest.java, > > line 143 > > <https://reviews.apache.org/r/17580/diff/1/?file=457525#file457525line143> > > > > generic name: falcon ? Its my signature. ;-) > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > messaging/src/test/java/org/apache/falcon/messaging/FalconTopicProducerTest.java, > > line 208 > > <https://reviews.apache.org/r/17580/diff/1/?file=457525#file457525line208> > > > > generic name: falcon ? Copy paste of signature. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > messaging/src/test/java/org/apache/falcon/messaging/FeedProducerTest.java, > > line 76 > > <https://reviews.apache.org/r/17580/diff/1/?file=457526#file457526line76> > > > > generic name: falcon ? ok, get it. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > messaging/src/test/java/org/apache/falcon/messaging/ProcessProducerTest.java, > > line 143 > > <https://reviews.apache.org/r/17580/diff/1/?file=457527#file457527line143> > > > > generic name: falcon ? You are now killing me. :-) > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > oozie/src/main/java/org/apache/falcon/converter/AbstractOozieEntityMapper.java, > > line 323 > > <https://reviews.apache.org/r/17580/diff/1/?file=457529#file457529line323> > > > > Can you please elaborate on what the gap is ? The logs are being copied from with in a oozie workflow running as the user and may be its by design right? I somehow think this is a hack. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > oozie/src/main/java/org/apache/falcon/logging/LogProvider.java, line 56 > > <https://reviews.apache.org/r/17580/diff/1/?file=457531#file457531line56> > > > > Is it required to proxy here ? Aren't logs written to a location that > > falcon owns with 777 permissions ? Ideally, this is owned by the user. But since falcon owns it and cleans it, may be it does not need to proxy in this case. Good catch. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > oozie/src/main/java/org/apache/falcon/workflow/FalconPostProcessing.java, > > line 166 > > <https://reviews.apache.org/r/17580/diff/1/?file=457533#file457533line166> > > > > I am assuming this is FALCON-222 Yes. But the kool thing is folks are working on this with in oozie. :-) > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > prism/src/main/java/org/apache/falcon/security/BasicAuthFilter.java, line 46 > > <https://reviews.apache.org/r/17580/diff/1/?file=457542#file457542line46> > > > > Might be handy to have more java docs in here to explain a new reader > > on what is the role of the Options servlet and how are things wired up Its all documented in Hadoop-auth. If you look at security docs, they contain a link. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > prism/src/main/java/org/apache/falcon/security/BasicAuthFilter.java, line 63 > > <https://reviews.apache.org/r/17580/diff/1/?file=457542#file457542line63> > > > > Sane defaults have been removed. Is it intentional ? Yes, this was too restrictive and hard to change. Instead, I have it wired up from the startup props file which is better. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > rerun/src/main/java/org/apache/falcon/latedata/LateDataHandler.java, line > > 139 > > <https://reviews.apache.org/r/17580/diff/1/?file=457549#file457549line139> > > > > Except for detectChanges() all other functions are invoked in the MR > > job, which is already running as the workflow user. This might not work Good catch, its unnecessary. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > rerun/src/main/java/org/apache/falcon/latedata/LateDataHandler.java, line > > 219 > > <https://reviews.apache.org/r/17580/diff/1/?file=457549#file457549line219> > > > > Except for detectChanges() all other functions are invoked in the MR > > job, which is already running as the workflow user. This might not work Good catch, its unnecessary. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > src/conf/log4j.xml, line 79 > > <https://reviews.apache.org/r/17580/diff/1/?file=457565#file457565line79> > > > > Why is this at trace level ? Testing security with out trace is horrendous. Will change that. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java, line 514 > > <https://reviews.apache.org/r/17580/diff/1/?file=457572#file457572line514> > > > > This section is commented out. Intentional? Not sure which one here. Line 508 does not have a comment. It it: Assert.assertEquals(0, new FalconCLI().run(("admin -stack -url " + TestContext.BASE_URL).split("\\s"))); - Seetharam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17580/#review33296 ----------------------------------------------------------- On Jan. 31, 2014, 5:27 a.m., Srikanth Sundarrajan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17580/ > ----------------------------------------------------------- > > (Updated Jan. 31, 2014, 5:27 a.m.) > > > Review request for Falcon. > > > Bugs: FALCON-11, FALCON-14, FALCON-15, FALCON-16, FALCON-17, FALCON-18, > FALCON-219, and FALCON-220 > https://issues.apache.org/jira/browse/FALCON-11 > https://issues.apache.org/jira/browse/FALCON-14 > https://issues.apache.org/jira/browse/FALCON-15 > https://issues.apache.org/jira/browse/FALCON-16 > https://issues.apache.org/jira/browse/FALCON-17 > https://issues.apache.org/jira/browse/FALCON-18 > https://issues.apache.org/jira/browse/FALCON-219 > https://issues.apache.org/jira/browse/FALCON-220 > > > Repository: falcon-git > > > Description > ------- > > FALCON-11 Add support for security in Falcon > > > Diffs > ----- > > checkstyle/src/main/resources/falcon/checkstyle.xml f6df5cd > client/pom.xml a43f7f5 > client/src/main/java/org/apache/falcon/client/FalconClient.java 4a7207f > common/pom.xml 068a22c > common/src/main/java/org/apache/falcon/catalog/AbstractCatalogService.java > 691d805 > common/src/main/java/org/apache/falcon/catalog/HiveCatalogService.java > 51e4d6e > common/src/main/java/org/apache/falcon/cleanup/AbstractCleanupHandler.java > 644afd2 > common/src/main/java/org/apache/falcon/cleanup/FeedCleanupHandler.java > 7dbac58 > common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 32f7605 > common/src/main/java/org/apache/falcon/entity/ClusterHelper.java 38b5c5b > common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java > 68370c7 > common/src/main/java/org/apache/falcon/entity/Storage.java 0634969 > > common/src/main/java/org/apache/falcon/entity/parser/ClusterEntityParser.java > e633838 > common/src/main/java/org/apache/falcon/entity/parser/FeedEntityParser.java > 5c1d9ad > > common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java > 8647d43 > common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java > 18ceb6e > common/src/main/java/org/apache/falcon/hadoop/HadoopClientFactory.java > PRE-CREATION > > common/src/main/java/org/apache/falcon/security/AuthenticationInitializationService.java > PRE-CREATION > common/src/main/java/org/apache/falcon/security/CurrentUser.java 4d2299e > common/src/main/java/org/apache/falcon/security/FalconLoginModule.java > d95e147 > > common/src/main/java/org/apache/falcon/security/FalconSecurityConfiguration.java > b80ab6d > common/src/main/java/org/apache/falcon/security/SecurityConstants.java > 8f7ba4a > common/src/main/java/org/apache/falcon/security/SecurityUtil.java > PRE-CREATION > common/src/main/java/org/apache/falcon/util/Preconditions.java PRE-CREATION > common/src/main/resources/startup.properties 3014418 > common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java 7668c7f > common/src/test/java/org/apache/falcon/entity/FileSystemStorageTest.java > 7b48d2b > common/src/test/java/org/apache/falcon/hadoop/HadoopClientFactoryTest.java > PRE-CREATION > > common/src/test/java/org/apache/falcon/security/AuthenticationInitializationServiceTest.java > PRE-CREATION > common/src/test/java/org/apache/falcon/security/SecurityUtilTest.java > PRE-CREATION > common/src/test/java/org/apache/falcon/util/PreconditionsTest.java > PRE-CREATION > feed/src/main/resources/config/workflow/replication-workflow.xml 91d0285 > feed/src/main/resources/config/workflow/retention-workflow.xml 8b444f5 > hadoop-webapp/pom.xml e576310 > messaging/pom.xml 9aa5347 > > messaging/src/main/java/org/apache/falcon/messaging/EntityInstanceMessage.java > eb49fd5 > > messaging/src/main/java/org/apache/falcon/messaging/EntityInstanceMessageCreator.java > ecda5eb > messaging/src/main/java/org/apache/falcon/messaging/MessageProducer.java > b37931c > > messaging/src/test/java/org/apache/falcon/messaging/FalconTopicProducerTest.java > da126c7 > messaging/src/test/java/org/apache/falcon/messaging/FeedProducerTest.java > e707567 > > messaging/src/test/java/org/apache/falcon/messaging/ProcessProducerTest.java > 3a40e76 > metrics/src/main/java/org/apache/falcon/aspect/GenericAlert.java 275a725 > > oozie/src/main/java/org/apache/falcon/converter/AbstractOozieEntityMapper.java > cecdeef > oozie/src/main/java/org/apache/falcon/logging/LogMover.java d544311 > oozie/src/main/java/org/apache/falcon/logging/LogProvider.java 48d4589 > > oozie/src/main/java/org/apache/falcon/service/SharedLibraryHostingService.java > 37f8cfa > oozie/src/main/java/org/apache/falcon/workflow/FalconPostProcessing.java > 3f9256c > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieClientFactory.java > b757531 > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieHouseKeepingService.java > 068e980 > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java > 7c3a425 > oozie/src/main/java/org/apache/oozie/client/CustomOozieClient.java c55221e > oozie/src/main/java/org/apache/oozie/client/ProxyOozieClient.java > PRE-CREATION > > oozie/src/test/java/org/apache/falcon/oozie/workflow/FalconPostProcessingTest.java > c6485cd > pom.xml 9252d80 > prism/src/main/java/org/apache/falcon/resource/channel/HTTPChannel.java > 2ba76a7 > prism/src/main/java/org/apache/falcon/security/BasicAuthFilter.java f172e82 > > prism/src/main/java/org/apache/falcon/security/RemoteUserInHeaderBasedAuthenticationHandler.java > PRE-CREATION > prism/src/main/java/org/apache/falcon/service/FalconTopicSubscriber.java > 6ac926d > prism/src/main/java/org/apache/falcon/service/ProcessSubscriberService.java > 1cd7776 > prism/src/test/java/org/apache/falcon/aspect/GenericAlertTest.java 1db71fd > > prism/src/test/java/org/apache/falcon/service/FalconTopicSubscriberTest.java > f1536f4 > process/src/main/resources/config/workflow/process-parent-workflow.xml > 494bf20 > rerun/src/main/java/org/apache/falcon/latedata/LateDataHandler.java 4b35760 > rerun/src/main/java/org/apache/falcon/rerun/event/LaterunEvent.java b5ac121 > rerun/src/main/java/org/apache/falcon/rerun/event/RerunEvent.java baf4601 > rerun/src/main/java/org/apache/falcon/rerun/event/RerunEventFactory.java > 03230f9 > rerun/src/main/java/org/apache/falcon/rerun/event/RetryEvent.java 1396f19 > > rerun/src/main/java/org/apache/falcon/rerun/handler/AbstractRerunConsumer.java > b073117 > > rerun/src/main/java/org/apache/falcon/rerun/handler/AbstractRerunHandler.java > ab7f472 > rerun/src/main/java/org/apache/falcon/rerun/handler/LateRerunConsumer.java > fffd5cd > rerun/src/main/java/org/apache/falcon/rerun/handler/LateRerunHandler.java > 897e7ab > rerun/src/main/java/org/apache/falcon/rerun/handler/RetryConsumer.java > 63dade8 > rerun/src/main/java/org/apache/falcon/rerun/handler/RetryHandler.java > 2b41a7c > rerun/src/main/java/org/apache/falcon/rerun/queue/InMemoryQueue.java > 8234d8a > > rerun/src/test/java/org/apache/falcon/rerun/handler/TestLateRerunHandler.java > e02b495 > rerun/src/test/java/org/apache/falcon/rerun/queue/ActiveMQTest.java 01d0415 > rerun/src/test/java/org/apache/falcon/rerun/queue/InMemoryQueueTest.java > 6aafaa5 > src/bin/falcon d196a5d > src/conf/log4j.xml 58ebd80 > src/conf/startup.properties 79cd211 > test-util/src/main/java/org/apache/falcon/cluster/util/EmbeddedCluster.java > 2b55407 > webapp/pom.xml 8c37409 > webapp/src/conf/oozie/conf/oozie-site.xml e5f404a > webapp/src/main/resources/log4j.xml d133b8e > webapp/src/test/java/org/apache/falcon/catalog/HiveCatalogServiceIT.java > 9909140 > webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java 72369c0 > webapp/src/test/java/org/apache/falcon/cli/FalconCLISmokeIT.java 55f240f > webapp/src/test/java/org/apache/falcon/late/LateDataHandlerIT.java 6cfa4e6 > > webapp/src/test/java/org/apache/falcon/lifecycle/FileSystemFeedReplicationIT.java > 058b35c > > webapp/src/test/java/org/apache/falcon/lifecycle/TableStorageFeedEvictorIT.java > 37226e2 > > webapp/src/test/java/org/apache/falcon/lifecycle/TableStorageFeedReplicationIT.java > dbc6442 > webapp/src/test/java/org/apache/falcon/process/PigProcessIT.java 1f4e9e8 > webapp/src/test/java/org/apache/falcon/process/TableStorageProcessIT.java > 2d539c2 > webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseyIT.java > 1ceaabf > > webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseySmokeIT.java > b96aa48 > > webapp/src/test/java/org/apache/falcon/resource/ProcessInstanceManagerIT.java > c0785ba > webapp/src/test/java/org/apache/falcon/resource/TestContext.java 9e10956 > webapp/src/test/java/org/apache/falcon/security/BasicAuthFilterTest.java > 0ff993b > webapp/src/test/java/org/apache/falcon/util/OozieTestUtils.java 3769dde > > webapp/src/test/java/org/apache/falcon/util/ResourcesReflectionUtilTest.java > e54f81c > > webapp/src/test/java/org/apache/falcon/validation/ClusterEntityValidationIT.java > 9299b5b > > webapp/src/test/java/org/apache/falcon/validation/FeedEntityValidationIT.java > db24b9a > > Diff: https://reviews.apache.org/r/17580/diff/ > > > Testing > ------- > > > Thanks, > > Srikanth Sundarrajan > >
