You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by HeartSaVioR <gi...@git.apache.org> on 2017/03/15 04:05:26 UTC
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/2005
STORM-2414 Skip checking ACL when clearing already removed blob from \u2026
\u2026local / HDFS blobstore
Syncing up blobs throws KeyNotFoundException when deleting blobs which are already deleted from ZK.
Currently deleting blob requires pulling the latest change first to check ACL. That makes sense, but in above case we already deleted the blob from ZK so it just fails. We need to have "force delete" when request for deletion comes from syncing up blobs.
* add new method: clearRemovedBlob
* skip checking ACL while deleting when blob is already removed from ZK
This patch is easy to be cherry-picked to master and 1.0.x branches as well.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-2414-1.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2005.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2005
----
commit d0ef90e24d567ccb87c5de545f5478a17e37f68e
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2017-03-15T03:47:14Z
STORM-2414 Skip checking ACL when clearing already removed blob from local / HDFS blobstore
* add new method: clearRemovedBlob
* skip checking ACL while deleting when blob is already removed from ZK
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2005: STORM-2414 Skip checking ACL when clearing already remove...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2005
Looking into the logic deeply and realize `deleteBlob` is not that simple as it seems especially with H/A... Investigating more on this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking meta's ACL when subject h...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106835526
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,37 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ if (!_aclHandler.checkForValidUsers(who, WRITE)) {
+ // need to get ACL from meta
+ checkForBlobOrDownload(key);
+ SettableBlobMeta meta = getStoredBlobMeta(key);
+ _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+ } else {
+ // able to delete the blob without checking meta's ACL
--- End diff --
LOG
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking meta's ACL when subject h...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106837869
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,37 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ if (!_aclHandler.checkForValidUsers(who, WRITE)) {
+ // need to get ACL from meta
--- End diff --
@arunmahadevan Will leave log message with debug level since it's useful for debugging. Please let me know if you would like to raise level to INFO or so.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106584346
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,44 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
+ } catch (KeyNotFoundException e) {
+ LOG.warn("Detected deletion from ZK for the key {} while pulling the latest change. Needs to be deleted. key: {}", key);
+ checkPermission = false;
+ }
+
+ if (checkPermission) {
+ try {
+ SettableBlobMeta meta = getStoredBlobMeta(key);
+ _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+ } catch (KeyNotFoundException e) {
+ // meta file is not exist, skip handling error to delete data file
--- End diff --
Thanks addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106349077
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,9 +219,20 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
--- End diff --
Sorry I was mistaken... Even if there's no key on ZK, KeyNotFoundException is not thrown.
KeyNotFoundException is thrown only when there's race condition between ZK node update and removing node from ZK.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106440281
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,44 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
+ } catch (KeyNotFoundException e) {
+ LOG.warn("Detected deletion from ZK for the key {} while pulling the latest change. Needs to be deleted. key: {}", key);
+ checkPermission = false;
+ }
+
+ if (checkPermission) {
+ try {
+ SettableBlobMeta meta = getStoredBlobMeta(key);
+ _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+ } catch (KeyNotFoundException e) {
+ // meta file is not exist, skip handling error to delete data file
--- End diff --
nit: meta file does not exist
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2005: STORM-2414 Skip checking meta's ACL when subject has writ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2005
Build failure is not related to this one: `storm-core` and `!storm-core` succeed for both JDK7 and JDK8.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking meta's ACL when subject h...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106837872
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,37 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ if (!_aclHandler.checkForValidUsers(who, WRITE)) {
+ // need to get ACL from meta
+ checkForBlobOrDownload(key);
+ SettableBlobMeta meta = getStoredBlobMeta(key);
+ _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+ } else {
+ // able to delete the blob without checking meta's ACL
--- End diff --
Same as above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106346237
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,9 +219,20 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
--- End diff --
Here KeyNotFoundException is thrown when blob is removed from ZK.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2005: STORM-2414 Skip checking ACL when clearing already remove...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/2005
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106342738
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/BlobStore.java ---
@@ -128,6 +128,15 @@
public abstract void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException;
/**
+ * Clear the blob data and metadata which is already removed from ZK.
+ * Implementation should just remove the blob as it is.
+ * This method should be used ONLY for syncing up blobs in Nimbus.
+ * @param key Key for the blob.
+ * @throws AuthorizationException
+ */
+ public abstract void clearRemovedBlob(String key) throws AuthorizationException;
--- End diff --
@satishd
Yes but no one would implement their own BlobStore.
Btw, as @arunmahadevan suggested, we can do it without this change. I'll change it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106346290
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,9 +219,20 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
+ } catch (KeyNotFoundException e) {
+ // not exist in ZK so this blob should be deleted regardless of permission
+ checkPermission = false;
+ }
+
+ if (checkPermission) {
+ SettableBlobMeta meta = getStoredBlobMeta(key);
--- End diff --
Here KeyNotFoundException is thrown when blob file is not found.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106334482
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/BlobStore.java ---
@@ -128,6 +128,15 @@
public abstract void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException;
/**
+ * Clear the blob data and metadata which is already removed from ZK.
+ * Implementation should just remove the blob as it is.
+ * This method should be used ONLY for syncing up blobs in Nimbus.
+ * @param key Key for the blob.
+ * @throws AuthorizationException
+ */
+ public abstract void clearRemovedBlob(String key) throws AuthorizationException;
--- End diff --
Is `BlobStore` a public API, which users can write their own implementations? If so, then this change would not be a backward compatible change for those users.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106455903
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,44 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
+ } catch (KeyNotFoundException e) {
+ LOG.warn("Detected deletion from ZK for the key {} while pulling the latest change. Needs to be deleted. key: {}", key);
+ checkPermission = false;
+ }
+
+ if (checkPermission) {
--- End diff --
Instead of this can you combine the two `try...catch` into something like
```java
try {
checkForBlobOrDownload(key);
SettableBlobMeta meta = getStoredBlobMeta(key);
_aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
} catch (KeyNotFoundException e) {
LOG.warn(...);
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2005: STORM-2414 Skip checking meta's ACL when subject has writ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2005
@arunmahadevan @satishd
I have updated the problem definition and fix for the problem.
If deleting blob is requested from superuser (say, Nimbus, Admin. Supervisor is not since Supervisor is only superuser for reading) we don't need to get meta since the user already get WRITE privileges for any blobs. Syncing up blobs is done via Nimbus privilege so it will pass downloading and/or reading meta.
If deleting blob is requested from user, the logic will be run via leader Nimbus. Given that the scenario, both `ZK node not found` and `meta file not found` are all errors which users (maybe admin) need to know about the thing.
Ideally we might want to consider repairing, but it makes the logic more complicated so I'd rather define Blobstore H/A as not fully reliable and recommend HDFS BlobStore for H/A environment.
Please take a look again. Thanks in advance!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking meta's ACL when subject h...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106837874
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,37 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ if (!_aclHandler.checkForValidUsers(who, WRITE)) {
+ // need to get ACL from meta
+ checkForBlobOrDownload(key);
+ SettableBlobMeta meta = getStoredBlobMeta(key);
+ _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+ } else {
+ // able to delete the blob without checking meta's ACL
+ // skip checking everything and continue deleting local files
+ }
+
try {
- fbs.deleteKey(DATA_PREFIX+key);
- fbs.deleteKey(META_PREFIX+key);
+ deleteKeyIgnoringFileNotFound(DATA_PREFIX + key);
+ deleteKeyIgnoringFileNotFound(META_PREFIX + key);
} catch (IOException e) {
throw new RuntimeException(e);
}
}
+ private void deleteKeyIgnoringFileNotFound(String key) throws IOException {
+ try {
+ fbs.deleteKey(key);
+ } catch (IOException e) {
+ if (e instanceof FileNotFoundException) {
+ // ignoring
--- End diff --
Same as above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106584890
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,9 +219,20 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
--- End diff --
Maybe we would need to check ZK and set checkPermission to false.
If there's race condition so ZK node didn't exist but added later, we might skip pulling latest information and delete the blob in local anyway, but it should be occurred only from standby Nimbus and the blob will be re-pulled at next sync up.
I'll make appropriate change. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106585741
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,44 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
+ } catch (KeyNotFoundException e) {
+ LOG.warn("Detected deletion from ZK for the key {} while pulling the latest change. Needs to be deleted. key: {}", key);
+ checkPermission = false;
+ }
+
+ if (checkPermission) {
--- End diff --
Unless you want to apply different logic in the catch block for the two methods. If we just want to log a generic message having single try..catch should be sufficient.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2005: STORM-2414 Skip checking ACL when clearing already remove...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2005
@harshach @satishd @arunmahadevan Addressed. Please review again. Thanks in advance!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2005: STORM-2414 Skip checking ACL when clearing already remove...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/2005
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106584243
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,44 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
+ } catch (KeyNotFoundException e) {
+ LOG.warn("Detected deletion from ZK for the key {} while pulling the latest change. Needs to be deleted. key: {}", key);
+ checkPermission = false;
+ }
+
+ if (checkPermission) {
--- End diff --
The meaning for KeyNotFoundException is different between `checkForBlobOrDownload` and `getStoredBlobMeta`. First one indicates ZK node is not exist and second one indicates META blob file in local is not exist.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking meta's ACL when subject h...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106835522
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,37 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ if (!_aclHandler.checkForValidUsers(who, WRITE)) {
+ // need to get ACL from meta
--- End diff --
add LOG
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106346689
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,9 +219,20 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
+ } catch (KeyNotFoundException e) {
+ // not exist in ZK so this blob should be deleted regardless of permission
--- End diff --
maybe just add some logs here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2005: STORM-2414 Skip checking ACL when clearing already remove...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2005
@arunmahadevan @satishd
Sorry please take a look again. Actually origin issue was observed when meta blob file is already deleted. (I've mistaken the codebase.) Changed to ignore the FNF exception when deleting file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106340804
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -231,6 +231,18 @@ public void deleteBlob(String key, Subject who) throws AuthorizationException, K
}
@Override
+ public void clearRemovedBlob(String key) throws AuthorizationException {
+ validateKey(key);
+ // not pulling recent change and checking ACL
--- End diff --
if you handle this in deleteBlob itself, we don't need to add new api to blobstore. i.e. if getStoredBlobMeta throws KeyNotFoundException, just log it and skip the ACL check and proceed with deleting the keys.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking meta's ACL when subject h...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2005
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2005: STORM-2414 Skip checking meta's ACL when subject has writ...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/2005
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106342906
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -231,6 +231,18 @@ public void deleteBlob(String key, Subject who) throws AuthorizationException, K
}
@Override
+ public void clearRemovedBlob(String key) throws AuthorizationException {
+ validateKey(key);
+ // not pulling recent change and checking ACL
--- End diff --
@arunmahadevan
Yes I was thinking about that way but over-thought about false-positive (when ZK is down, throwing Exception, etc). KeyNotFoundException clearly states that ZK is accessible and key is not exist so no false-positive. Thanks I'll address this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking meta's ACL when subject h...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106835544
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,17 +219,37 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ if (!_aclHandler.checkForValidUsers(who, WRITE)) {
+ // need to get ACL from meta
+ checkForBlobOrDownload(key);
+ SettableBlobMeta meta = getStoredBlobMeta(key);
+ _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+ } else {
+ // able to delete the blob without checking meta's ACL
+ // skip checking everything and continue deleting local files
+ }
+
try {
- fbs.deleteKey(DATA_PREFIX+key);
- fbs.deleteKey(META_PREFIX+key);
+ deleteKeyIgnoringFileNotFound(DATA_PREFIX + key);
+ deleteKeyIgnoringFileNotFound(META_PREFIX + key);
} catch (IOException e) {
throw new RuntimeException(e);
}
}
+ private void deleteKeyIgnoringFileNotFound(String key) throws IOException {
+ try {
+ fbs.deleteKey(key);
+ } catch (IOException e) {
+ if (e instanceof FileNotFoundException) {
+ // ignoring
--- End diff --
LOG
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106439220
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -219,9 +219,20 @@ public void setBlobMeta(String key, SettableBlobMeta meta, Subject who) throws A
@Override
public void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException {
validateKey(key);
- checkForBlobOrDownload(key);
- SettableBlobMeta meta = getStoredBlobMeta(key);
- _aclHandler.hasPermissions(meta.get_acl(), WRITE, who, key);
+
+ boolean checkPermission = true;
+ try {
+ checkForBlobOrDownload(key);
--- End diff --
If it does not throw `KeyNotFoundException` when there is no key on ZK then how do you find out whether the given key exists or not on ZK so that you can set `checkPermission` as `false`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2005: STORM-2414 Skip checking ACL when clearing already...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/2005#discussion_r106343405
--- Diff: storm-core/src/jvm/org/apache/storm/blobstore/BlobStore.java ---
@@ -128,6 +128,15 @@
public abstract void deleteBlob(String key, Subject who) throws AuthorizationException, KeyNotFoundException;
/**
+ * Clear the blob data and metadata which is already removed from ZK.
+ * Implementation should just remove the blob as it is.
+ * This method should be used ONLY for syncing up blobs in Nimbus.
+ * @param key Key for the blob.
+ * @throws AuthorizationException
+ */
+ public abstract void clearRemovedBlob(String key) throws AuthorizationException;
--- End diff --
@HeartSaVioR :) May not want to assume no one implements their store when it is a public API. Anyways, good that `deleteBlob` can be changed to handle this scenario.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---