You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org> on 2018/09/04 16:42:46 UTC
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/
-----------------------------------------------------------
(Updated Sept. 4, 2018, 4:42 p.m.)
Review request for sentry and Arjun Mishra.
Bugs: SENTRY-2287
https://issues.apache.org/jira/browse/SENTRY-2287
Repository: sentry
Description
-------
Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
Diffs
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 0d39300fe0fddd205e5a1ed868ee818475628132
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2b1618134921a594e137a0339cf517f7ccd9bc03
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java b9405ccd23594db6218af2cd184c82ce59ae5ec4
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java f3a2d5028a3e429b450894b3fe12526a1392e40a
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 5e2d5c5ee6bd5a65aebc6d00e6e3f4a506cf2b07
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
Diff: https://reviews.apache.org/r/68622/diff/1/
Testing
-------
Thanks,
kalyan kumar kalvagadda
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208370
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 104 (original), 104 (patched)
<https://reviews.apache.org/r/68622/#comment292256>
can you change the wording as
"(%s) is sending full update to HDFS, Reason: newer snapshot with id:(%s) available than requested one (%s)"
- Na Li
On Sept. 4, 2018, 4:42 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2018, 4:42 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 0d39300fe0fddd205e5a1ed868ee818475628132
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2b1618134921a594e137a0339cf517f7ccd9bc03
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java b9405ccd23594db6218af2cd184c82ce59ae5ec4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java f3a2d5028a3e429b450894b3fe12526a1392e40a
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 5e2d5c5ee6bd5a65aebc6d00e6e3f4a506cf2b07
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68622/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On Sept. 5, 2018, 8:27 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line74>
> >
> > we should seperate the logic for path update from perm update. They can be independent.
They need not be because sentry namenode plugin requets for both paths and permissions in a single request and sentry responds back with pahs and permissions in the same response back.
> On Sept. 5, 2018, 8:27 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 75 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line75>
> >
> > this line is the same as line 74. Why do you duplicate it?
There was a typo. Good find.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208371
-----------------------------------------------------------
On Jan. 24, 2019, 6:38 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2019, 6:38 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 81c614a345c122e067ec0a19b8f75766390b2ad4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 3532ef33dde8b119ab095861381fa52fa5520f4c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 8d6713acd61f99940f72c4985098dfeabb9fc832
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 443434127b14fadaeb27717ad2370dcdc10ca70c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 0cd405b54ee2b4bf788dffb1ac606362614f6efe
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java 2afe919c31b0c5f495d54c6448593abad38eddc2
>
>
> Diff: https://reviews.apache.org/r/68622/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208371
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 145 (original), 144 (patched)
<https://reviews.apache.org/r/68622/#comment292257>
change the message to
String.format("(%s) is sending full update to HDFS, Reason: Requested Seq num:(%d) is not available in deltas", retrieverType, seqNum)
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
Lines 52 (patched)
<https://reviews.apache.org/r/68622/#comment292258>
since you enable the state here, should you disable the state once you are done?
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 54 (patched)
<https://reviews.apache.org/r/68622/#comment292263>
disable state once fetching is done
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
Lines 53 (patched)
<https://reviews.apache.org/r/68622/#comment292262>
disable state once fetching is done
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
Lines 57 (patched)
<https://reviews.apache.org/r/68622/#comment292261>
should disable the state once fetching is done.
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
Lines 74 (patched)
<https://reviews.apache.org/r/68622/#comment292260>
we should seperate the logic for path update from perm update. They can be independent.
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
Lines 75 (patched)
<https://reviews.apache.org/r/68622/#comment292259>
this line is the same as line 74. Why do you duplicate it?
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
Lines 195 (patched)
<https://reviews.apache.org/r/68622/#comment292264>
you should disable the state SENDING_FULL_UPDATES when you get client request for delta change.
Have you realized the state is stored as bitmap? So multiple states can co-exist. You have to disable the state explicitly.
In SentryStateBank
public static void enableState(String component, SentryState state) {
lock.writeLock().lock();
try {
states.put(component, states.get(component) | state.getValue()); <- stored as bitmap. Old value is not removed
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("{} entered state {}", component, state.toString());
}
} finally {
lock.writeLock().unlock();
}
}
- Na Li
On Sept. 4, 2018, 4:42 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2018, 4:42 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 0d39300fe0fddd205e5a1ed868ee818475628132
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2b1618134921a594e137a0339cf517f7ccd9bc03
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java b9405ccd23594db6218af2cd184c82ce59ae5ec4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java f3a2d5028a3e429b450894b3fe12526a1392e40a
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 5e2d5c5ee6bd5a65aebc6d00e6e3f4a506cf2b07
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68622/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line74>
> >
> > This condition should be after getUpdatesFrom methods for path and perm are called. That is where state SENDING_FULL_UPDATES is set.
> >
> > Also this should be an OR for Path Updates or Perm updates. We are checking for sending full update state as opposed to the inital HDFS ACL sync
This logic is hit when the client sends a request after sentry server sent the full update for perms and paths.
> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 76 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line76>
> >
> > Do we need the isClientRequestingNextSeqId check? The current states should be enough
This logic is hit when the client sends a request after sentry server sent the full update for perms and paths. This check will make sure that client has succefully processed the full update and is asking for new updates.
> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 77 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line77>
> >
> > This log message should be sending full path/permission updates as opposed to ACL synchorication complete. Also it should be after the retVal.set method
Please check method "updateState" which is invoked in getPathsUpdatesFrom and getPermissionsUpdatesFrom where the state is set to SENDING_FULL_UPDATES.
> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 215 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line215>
> >
> > Path and Perm sequence won't necessarily have a difference of 1. SENTRY_PERM_CHANGE and SENTRY_PATH_CHANGE tables are cleaned and the sequence gap can be larger.
> >
> > I think we can get rid of this method isClientRequestingNextSeqId and be completely dependent on state
Client always requests the last seq number it received + 1.
SentryAuthzUpdate getUpdates() {
if (sentryClient == null) {
try {
sentryClient = SentryHDFSServiceClientFactory.create(conf);
} catch (Exception e) {
LOG.error("Error connecting to Sentry !!",
e.getMessage());
sentryClient = null;
return null;
}
}
try {
return sentryClient.getAllUpdatesFrom(
authzInfo.getAuthzPermissions().getLastUpdatedSeqNum() + 1,
authzInfo.getAuthzPaths().getLastUpdatedSeqNum() + 1,
authzInfo.getAuthzPaths().getLastUpdatedImgNum());
} catch (Exception e) {
sentryClient = null;
LOG.error("Error receiving updates from Sentry", e);
return null;
}
}
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208319
-----------------------------------------------------------
On Sept. 4, 2018, 4:42 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2018, 4:42 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 0d39300fe0fddd205e5a1ed868ee818475628132
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2b1618134921a594e137a0339cf517f7ccd9bc03
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java b9405ccd23594db6218af2cd184c82ce59ae5ec4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java f3a2d5028a3e429b450894b3fe12526a1392e40a
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 5e2d5c5ee6bd5a65aebc6d00e6e3f4a506cf2b07
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68622/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line74>
> >
> > This condition should be after getUpdatesFrom methods for path and perm are called. That is where state SENDING_FULL_UPDATES is set.
> >
> > Also this should be an OR for Path Updates or Perm updates. We are checking for sending full update state as opposed to the inital HDFS ACL sync
>
> kalyan kumar kalvagadda wrote:
> This logic is hit when the client sends a request after sentry server sent the full update for perms and paths.
>
> Arjun Mishra wrote:
> Ok got it. So it is logged on the cosecutive call. I still see a problem though. I think its not fair to say PathUpdate and PermUpdate will be set to SENDING_FULL_UPDATES at the same time. Your logic is assuming that current path and perm update state is SENDING_FULL_UPDATES but it can be the case that PathUpdate is still at FETCHING_FULL_UPDATES, while PermUpdate state (which usually occurs faster) has completed sending full updates and is now at SENDING_DELTA_UPDATE. That means when PathUpdate completes sending full update, this condition will not be true
> (SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, PermUpdaterState.SENDING_FULL_UPDATES) && SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, PermUpdaterState.SENDING_FULL_UPDATES))
>
> Arjun Mishra wrote:
> I think instead of verifying if HDFS ACL sync is complete, we can only log if Path Full Update has been sent, and Perm Full Update has been sent.
The corrent logic should actually be if both Path and Perm update state is at SENDING_DELTA_UPDATES. That is a garuntee that full updates have already been sent, since deltaas are requested only after full updates were sent
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208319
-----------------------------------------------------------
On Sept. 4, 2018, 4:42 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2018, 4:42 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 0d39300fe0fddd205e5a1ed868ee818475628132
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2b1618134921a594e137a0339cf517f7ccd9bc03
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java b9405ccd23594db6218af2cd184c82ce59ae5ec4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java f3a2d5028a3e429b450894b3fe12526a1392e40a
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 5e2d5c5ee6bd5a65aebc6d00e6e3f4a506cf2b07
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68622/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208319
-----------------------------------------------------------
On Jan. 24, 2019, 6:38 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2019, 6:38 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 81c614a345c122e067ec0a19b8f75766390b2ad4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 3532ef33dde8b119ab095861381fa52fa5520f4c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 8d6713acd61f99940f72c4985098dfeabb9fc832
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 443434127b14fadaeb27717ad2370dcdc10ca70c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 0cd405b54ee2b4bf788dffb1ac606362614f6efe
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java 2afe919c31b0c5f495d54c6448593abad38eddc2
>
>
> Diff: https://reviews.apache.org/r/68622/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line74>
> >
> > This condition should be after getUpdatesFrom methods for path and perm are called. That is where state SENDING_FULL_UPDATES is set.
> >
> > Also this should be an OR for Path Updates or Perm updates. We are checking for sending full update state as opposed to the inital HDFS ACL sync
>
> kalyan kumar kalvagadda wrote:
> This logic is hit when the client sends a request after sentry server sent the full update for perms and paths.
>
> Arjun Mishra wrote:
> Ok got it. So it is logged on the cosecutive call. I still see a problem though. I think its not fair to say PathUpdate and PermUpdate will be set to SENDING_FULL_UPDATES at the same time. Your logic is assuming that current path and perm update state is SENDING_FULL_UPDATES but it can be the case that PathUpdate is still at FETCHING_FULL_UPDATES, while PermUpdate state (which usually occurs faster) has completed sending full updates and is now at SENDING_DELTA_UPDATE. That means when PathUpdate completes sending full update, this condition will not be true
> (SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, PermUpdaterState.SENDING_FULL_UPDATES) && SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, PermUpdaterState.SENDING_FULL_UPDATES))
>
> Arjun Mishra wrote:
> I think instead of verifying if HDFS ACL sync is complete, we can only log if Path Full Update has been sent, and Perm Full Update has been sent.
>
> Arjun Mishra wrote:
> The corrent logic should actually be if both Path and Perm update state is at SENDING_DELTA_UPDATES. That is a garuntee that full updates have already been sent, since deltaas are requested only after full updates were sent
Here is the logic makes sure that thar both PathUpdaterState and PermUpdaterState are in SENDING_FULL_UPDATES and there was a request from Namenode asking for path and perm sequences what which are greater than the sequence numbers that were served.
If we change the logic as you suggested it could take longer as there need not be updates after a full update was sent.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208319
-----------------------------------------------------------
On Jan. 24, 2019, 6:38 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2019, 6:38 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 81c614a345c122e067ec0a19b8f75766390b2ad4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 3532ef33dde8b119ab095861381fa52fa5520f4c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 8d6713acd61f99940f72c4985098dfeabb9fc832
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 443434127b14fadaeb27717ad2370dcdc10ca70c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 0cd405b54ee2b4bf788dffb1ac606362614f6efe
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java 2afe919c31b0c5f495d54c6448593abad38eddc2
>
>
> Diff: https://reviews.apache.org/r/68622/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line74>
> >
> > This condition should be after getUpdatesFrom methods for path and perm are called. That is where state SENDING_FULL_UPDATES is set.
> >
> > Also this should be an OR for Path Updates or Perm updates. We are checking for sending full update state as opposed to the inital HDFS ACL sync
>
> kalyan kumar kalvagadda wrote:
> This logic is hit when the client sends a request after sentry server sent the full update for perms and paths.
>
> Arjun Mishra wrote:
> Ok got it. So it is logged on the cosecutive call. I still see a problem though. I think its not fair to say PathUpdate and PermUpdate will be set to SENDING_FULL_UPDATES at the same time. Your logic is assuming that current path and perm update state is SENDING_FULL_UPDATES but it can be the case that PathUpdate is still at FETCHING_FULL_UPDATES, while PermUpdate state (which usually occurs faster) has completed sending full updates and is now at SENDING_DELTA_UPDATE. That means when PathUpdate completes sending full update, this condition will not be true
> (SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, PermUpdaterState.SENDING_FULL_UPDATES) && SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, PermUpdaterState.SENDING_FULL_UPDATES))
I think instead of verifying if HDFS ACL sync is complete, we can only log if Path Full Update has been sent, and Perm Full Update has been sent.
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208319
-----------------------------------------------------------
On Sept. 4, 2018, 4:42 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2018, 4:42 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 0d39300fe0fddd205e5a1ed868ee818475628132
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2b1618134921a594e137a0339cf517f7ccd9bc03
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java b9405ccd23594db6218af2cd184c82ce59ae5ec4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java f3a2d5028a3e429b450894b3fe12526a1392e40a
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 5e2d5c5ee6bd5a65aebc6d00e6e3f4a506cf2b07
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68622/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/68622/diff/1/?file=2082214#file2082214line74>
> >
> > This condition should be after getUpdatesFrom methods for path and perm are called. That is where state SENDING_FULL_UPDATES is set.
> >
> > Also this should be an OR for Path Updates or Perm updates. We are checking for sending full update state as opposed to the inital HDFS ACL sync
>
> kalyan kumar kalvagadda wrote:
> This logic is hit when the client sends a request after sentry server sent the full update for perms and paths.
Ok got it. So it is logged on the cosecutive call. I still see a problem though. I think its not fair to say PathUpdate and PermUpdate will be set to SENDING_FULL_UPDATES at the same time. Your logic is assuming that current path and perm update state is SENDING_FULL_UPDATES but it can be the case that PathUpdate is still at FETCHING_FULL_UPDATES, while PermUpdate state (which usually occurs faster) has completed sending full updates and is now at SENDING_DELTA_UPDATE. That means when PathUpdate completes sending full update, this condition will not be true
(SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, PermUpdaterState.SENDING_FULL_UPDATES) && SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, PermUpdaterState.SENDING_FULL_UPDATES))
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208319
-----------------------------------------------------------
On Sept. 4, 2018, 4:42 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2018, 4:42 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 0d39300fe0fddd205e5a1ed868ee818475628132
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2b1618134921a594e137a0339cf517f7ccd9bc03
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java b9405ccd23594db6218af2cd184c82ce59ae5ec4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java f3a2d5028a3e429b450894b3fe12526a1392e40a
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 5e2d5c5ee6bd5a65aebc6d00e6e3f4a506cf2b07
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68622/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/#review208319
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
Lines 74 (patched)
<https://reviews.apache.org/r/68622/#comment292151>
This condition should be after getUpdatesFrom methods for path and perm are called. That is where state SENDING_FULL_UPDATES is set.
Also this should be an OR for Path Updates or Perm updates. We are checking for sending full update state as opposed to the inital HDFS ACL sync
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
Lines 76 (patched)
<https://reviews.apache.org/r/68622/#comment292152>
Do we need the isClientRequestingNextSeqId check? The current states should be enough
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
Lines 77 (patched)
<https://reviews.apache.org/r/68622/#comment292150>
This log message should be sending full path/permission updates as opposed to ACL synchorication complete. Also it should be after the retVal.set method
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
Lines 215 (patched)
<https://reviews.apache.org/r/68622/#comment292153>
Path and Perm sequence won't necessarily have a difference of 1. SENTRY_PERM_CHANGE and SENTRY_PATH_CHANGE tables are cleaned and the sequence gap can be larger.
I think we can get rid of this method isClientRequestingNextSeqId and be completely dependent on state
- Arjun Mishra
On Sept. 4, 2018, 4:42 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2018, 4:42 p.m.)
>
>
> Review request for sentry and Arjun Mishra.
>
>
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 0d39300fe0fddd205e5a1ed868ee818475628132
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2b1618134921a594e137a0339cf517f7ccd9bc03
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java b9405ccd23594db6218af2cd184c82ce59ae5ec4
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java f3a2d5028a3e429b450894b3fe12526a1392e40a
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 5e2d5c5ee6bd5a65aebc6d00e6e3f4a506cf2b07
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68622/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS
ACL synchronization is complete.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68622/
-----------------------------------------------------------
(Updated Jan. 24, 2019, 6:38 p.m.)
Review request for sentry and Arjun Mishra.
Changes
-------
addressed review comments.
Bugs: SENTRY-2287
https://issues.apache.org/jira/browse/SENTRY-2287
Repository: sentry
Description
-------
Currently there is no way to confirm that HDFS ACL synchronization is complete when snapshot is initiated. We need to identify that and log in console and log file as well.
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 08b16a4df3ea9126f21248365d6096fcdb83f21e
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java 81c614a345c122e067ec0a19b8f75766390b2ad4
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 3532ef33dde8b119ab095861381fa52fa5520f4c
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 8d6713acd61f99940f72c4985098dfeabb9fc832
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 443434127b14fadaeb27717ad2370dcdc10ca70c
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java 0cd405b54ee2b4bf788dffb1ac606362614f6efe
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java 2afe919c31b0c5f495d54c6448593abad38eddc2
Diff: https://reviews.apache.org/r/68622/diff/2/
Changes: https://reviews.apache.org/r/68622/diff/1-2/
Testing
-------
Thanks,
kalyan kumar kalvagadda