Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

2018-02-15 Thread kalyan kumar kalvagadda via Review Board 


> On Feb. 7, 2018, 5:02 a.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
> > Lines 125 (patched)
> > 
> >
> > Should you update the following condition for 
> > DBUpdateForwarder.getAllUpdatesFrom()?
> > 
> > Replace
> > if (seqNum > curSeqNum) {
> >   // No new notifications were processed.
> >   return Collections.emptyList();
> > }
> > 
> >  With
> >  if (seqNum > curSeqNum) {
> >  if(seqNum = curSeqNum + 1) {
> >   // No new notifications were processed.
> >   return Collections.emptyList();
> >   } else {
> >// perm change has been reset, get full permsnapshot 
> >ListfullImage = retrieveFullImage();
> > //Only log if we have received full image
> > if( !fullImage.isEmpty()) {
> >   LOGGER.info("({}) A newer full update with image number {} 
> > was found and being sent to HDFS",
> >   retrieverType, curImgNum);
> > }
> > return fullImage;
> >   }
> > }
> > 
> >  Once you clear the perm change, the seqNum from Name Node will be 
> > larger than curSeqNum. The correct behavior should be to send full snapshot 
> > of permission to Name Node after you clear the perm changes.
> 
> Na Li wrote:
> Another scenario to take care is that: 1) Name Node has synced with full 
> snapshot before HDFS sync is disabled and is asking for seqNum = 1 after HDFS 
> sync is enabled. A full snapshot should be sent to Name Node, but based on 
> above logic, when there is no perm changes, empty list is sent back.
> 
> kalyan kumar kalvagadda wrote:
> Enabling/disabling HDFS sync is done by changing an bunch of properties 
> both in sentry server and namnode. When even it happens both sentry and 
> namenode are restarted.
> 
> When ever namenode starts, it starts from fresh and the seqNum will be 0.
> 
> I'm dropping your comment. If you don't agree, plese re-open it with your 
> comment.
> 
> Na Li wrote:
> Sentry service has no control on Name Node service. When sentry 
> configuration changes, it is not guaranteed that Name Node service will be 
> restarted. I am OK if you 
> 1) add another jira to properly address the issue I mentioned above 
> (optional)
> 2) add comment in code for this assumption 
> 3) add the document in sentry for this assumption
> and finish this jira. 
> 
> It is a dangerous design to assume a behavior that is not guaranteed in 
> order for the feature to work properly.

This will not be an issue with the new patch


- kalyan kumar


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


On Feb. 15, 2018, 9 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65533/
> ---
> 
> (Updated Feb. 15, 2018, 9 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2115
> https://issues.apache.org/jira/browse/SENTRY-2115
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> **Scenario-1:** When HDFS sync is disabled, and sentry is started for the 
> first time.
> **Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is 
> less than last event-d processed by sentry
> **Scenario-3:** When HDFS sync is disabled, and first event-id in the 
> subsequent pull is not greater than the last event-id processed by sentry by 
> 1.
> **New Behavior:** Full snapshots need not be taken in all
> When Sentry detects out-of-sync situations, it should reset 
> SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
> HMS_NOTIFICATION_LOG from beginning.
> 
> **Scenario-4:** Initially HDFS sync was enabled and later disabled for while 
> and then HDFS sync is enabled and sentry service is restarted to get it to 
> effect.
> **New Behavior:** When Sentry detects out-of-sync situations, it should reset 
> SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
> HMS_NOTIFICATION_LOG from beginning.
> To handle scenario explained in Scenario-4, sentry should reset the mapping 
> information when ever HDFS sync is disabled. That way it can learn from 
> scratch when the feature is enabled back. There is no value is holding stale 
> data even when we know it will have issues when the feature is enabled back.
> 
> 
> Diffs
> -
> 
>   
>

Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

2018-02-15 Thread kalyan kumar kalvagadda via Review Board 

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

(Updated Feb. 15, 2018, 9 p.m.)


Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.


Changes
---

Addressed comments from sergio and lina.


Bugs: SENTRY-2115
https://issues.apache.org/jira/browse/SENTRY-2115


Repository: sentry


Description
---

**Scenario-1:** When HDFS sync is disabled, and sentry is started for the first 
time.
**Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is 
less than last event-d processed by sentry
**Scenario-3:** When HDFS sync is disabled, and first event-id in the 
subsequent pull is not greater than the last event-id processed by sentry by 1.
**New Behavior:** Full snapshots need not be taken in all
When Sentry detects out-of-sync situations, it should reset 
SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
HMS_NOTIFICATION_LOG from beginning.

**Scenario-4:** Initially HDFS sync was enabled and later disabled for while 
and then HDFS sync is enabled and sentry service is restarted to get it to 
effect.
**New Behavior:** When Sentry detects out-of-sync situations, it should reset 
SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
HMS_NOTIFICATION_LOG from beginning.
To handle scenario explained in Scenario-4, sentry should reset the mapping 
information when ever HDFS sync is disabled. That way it can learn from scratch 
when the feature is enabled back. There is no value is holding stale data even 
when we know it will have issues when the feature is enabled back.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 eae7861728f2bc11b4c1b44aa3b61b881a87740b 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 cf764eda1a006ce96f301e3ecb87749e05ba4a09 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 2f2b98412e7dfdcc847ffe7975a70f452554e747 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 e7558370025c6acd83492615be093f2bd16a202b 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 96c6810baa4d554db2b7d3739a28e3ff7e8b33a0 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 79030780c35e5bda432e3ec3f01328e627cb50a6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4cd00e6672730773c74b9840247d1f4d5f7bdfe4 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 e64f5cd687bf59133d6475c912ebdd7930601151 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
 91397061e78a6524410d35643dd3a33be8353ecc 


Diff: https://reviews.apache.org/r/65533/diff/3/

Changes: https://reviews.apache.org/r/65533/diff/2-3/


Testing
---

Made sure that all the tests passed.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

2018-02-15 Thread Xinran Tinney 


> On Jan. 11, 2018, 11:19 p.m., Sergio Pena wrote:
> > It looks good.
> > 
> > Did you test that all the commands which use SentryMain work correctly? 
> > bin/sentry, bin/run_sentry.sh, bin/config_tool?
> 
> Xinran Tinney wrote:
> I have not, how to verify it is correct? just run .sh?

I have run "./run_sentry.sh --command  schema-tool --conffile sentry-site.xml 
--dbType mysql --initSchema" under the patch and it shows the same result as 
without the patch on sentry master: 
[WARNING] Couldn't destroy threadgroup 
org.codehaus.mojo.exec.ExecJavaMojo$IsolatedThreadGroup[name=org.apache.sentry.SentryMain,maxpri=10]
java.lang.IllegalThreadStateException
at java.lang.ThreadGroup.destroy(ThreadGroup.java:778)
at org.codehaus.mojo.exec.ExecJavaMojo.execute(ExecJavaMojo.java:321)
at 
org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134)
at 
org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:207)
at 
org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:153)
at 
org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:145)
at 
org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:116)
at 
org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:80)
at 
org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:51)
at 
org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:128)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:307)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:193)
at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:106)
at org.apache.maven.cli.MavenCli.execute(MavenCli.java:863)
at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:288)
at org.apache.maven.cli.MavenCli.main(MavenCli.java:199)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:289)
at 
org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:229)
at 
org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:415)
at 
org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:356)
[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 18.759 s
[INFO] Finished at: 2018-02-15T09:41:35-08:00
[INFO] Final Memory: 44M/609M
[INFO] 


- Xinran


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


On Jan. 10, 2018, 9:38 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> ---
> 
> (Updated Jan. 10, 2018, 9:38 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TheSentryMain class currently works by mapping the command name to a Java 
> class that is then dynamically loaded:
> String commandName = commandLine.getOptionValue(COMMAND);
> String commandClazz = COMMANDS.get(commandName);
> Object command;
> try {
>   command = Class.forName(commandClazz.trim()).newInstance();
> } catch (Exception e) {
>   String msg = "Could not create instance of " + commandClazz + " for 
> command " + commandName;
>   throw new IllegalStateException(msg, e);
> }
> if (!(command instanceof Command)) {
>   String msg = "Command " + command.getClass().getName() + " is not an 
> instance of "
>   + Command.class.getName();
>   throw new IllegalStateException(msg);
> }
> ((Command)command).run(commandLine.getArgs());
>   }
> This ia too complicated and causes subtle problems at runtime. Instead it 
> should just create a new instance of appropriate class and call it directly.
> 
> 
> Diffs
> -
> 
>   bin/sentry 54e545aa 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
>  3a981b2a 
>