You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by revans2 <gi...@git.apache.org> on 2018/08/23 16:24:44 UTC
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2812
STORM-3203: add back in the permission updates
This adds back in some permission updates and a directory creation that was missed previously.
I have manually tested it and it works.
The old code is at
https://github.com/apache/storm/blob/d2d6f40344e6cc92ab07f3a462d577ef6b61f8b1/storm-core/src/jvm/org/apache/storm/localizer/AsyncLocalizer.java#L101-L165
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator-storm STORM-3203
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2812.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 #2812
----
commit 181d977362465ee9f84f63f7b0cfc575389aba31
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-08-23T15:45:57Z
STORM-3203: add back in the permission updates
----
---
[GitHub] storm issue #2812: STORM-3203: add back in the permission updates
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2812
@danny0405 @HeartSaVioR I addressed the review comments. Please let me know if there is something else.
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r212838182
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -53,21 +54,24 @@
private final boolean isLocalMode;
private final Path topologyBasicBlobsRootDir;
private final AdvancedFSOps fsOps;
+ private final String owner;
private volatile long version = NOT_DOWNLOADED_VERSION;
private volatile long size = 0;
+
/**
* Create a new LocallyCachedBlob.
- *
- * @param topologyId the ID of the topology.
+ * @param topologyId the ID of the topology.
* @param type the type of the blob.
+ * @param assignment the assignment, mostly to know who the owner is.
*/
protected LocallyCachedTopologyBlob(final String topologyId, final boolean isLocalMode, final Map<String, Object> conf,
- final AdvancedFSOps fsOps, final TopologyBlobType type) throws IOException {
+ final AdvancedFSOps fsOps, final TopologyBlobType type, LocalAssignment assignment) throws IOException {
super(topologyId + " " + type.getFileName(), type.getKey(topologyId));
this.topologyId = topologyId;
this.type = type;
this.isLocalMode = isLocalMode;
this.fsOps = fsOps;
+ this.owner = assignment.get_owner();
topologyBasicBlobsRootDir = Paths.get(ConfigUtils.supervisorStormDistRoot(conf, topologyId));
--- End diff --
+1 on this, but not a big deal. Just curious on my side too.
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r212851256
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -125,6 +129,12 @@ public long getRemoteVersion(ClientBlobStore store) throws KeyNotFoundException,
@Override
public long fetchUnzipToTemp(ClientBlobStore store)
throws IOException, KeyNotFoundException, AuthorizationException {
+ synchronized (LocallyCachedTopologyBlob.class) {
+ if (!Files.exists(topologyBasicBlobsRootDir)) {
--- End diff --
If we enforce different dirs for supervisors on one machine, i'm okey with this.😀
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r212787574
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -230,6 +240,19 @@ public void commitNewVersion(long newVersion) throws IOException {
//Don't try to move the JAR file in local mode, it does not exist because it was not uploaded
Files.move(tempLoc, dest);
}
+ synchronized (LocallyCachedTopologyBlob.class) {
+ //This is a bit ugly, but it works. In order to maintain the same directory structure that existed before
+ // we need to have storm conf, storm jar, and storm code in a shared directory, and we need to set the
+ // permissions for that entire directory, but the tracking is on a per item basis, so we are going to end
+ // up running the permission modification code once for each blob that is downloaded (3 times in this case).
+ // Because the permission modification code runs in a separate process we are doing a global lock to avoid
+ // any races between multiple versions running at the same time. Ideally this would be on a per topology
+ // basis, but that is a lot harder and the changes run fairly quickly so it should not be a big deal.
+ fsOps.setupStormCodeDir(owner, topologyBasicBlobsRootDir.toFile());
--- End diff --
It's not a big deal if we modify permission for 3 times of every topology if they are write conflict safe. Agree with you that a Supervisor scope lock would be easier to do, but if we have multi supervisors, the write conflict should be considered.
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r213030220
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -53,21 +54,24 @@
private final boolean isLocalMode;
private final Path topologyBasicBlobsRootDir;
private final AdvancedFSOps fsOps;
+ private final String owner;
private volatile long version = NOT_DOWNLOADED_VERSION;
private volatile long size = 0;
+
/**
* Create a new LocallyCachedBlob.
- *
- * @param topologyId the ID of the topology.
+ * @param topologyId the ID of the topology.
* @param type the type of the blob.
+ * @param assignment the assignment, mostly to know who the owner is.
*/
protected LocallyCachedTopologyBlob(final String topologyId, final boolean isLocalMode, final Map<String, Object> conf,
- final AdvancedFSOps fsOps, final TopologyBlobType type) throws IOException {
+ final AdvancedFSOps fsOps, final TopologyBlobType type, LocalAssignment assignment) throws IOException {
super(topologyId + " " + type.getFileName(), type.getKey(topologyId));
this.topologyId = topologyId;
this.type = type;
this.isLocalMode = isLocalMode;
this.fsOps = fsOps;
+ this.owner = assignment.get_owner();
topologyBasicBlobsRootDir = Paths.get(ConfigUtils.supervisorStormDistRoot(conf, topologyId));
--- End diff --
So originally the code we had internally was using the assignment to decide if it needed to create the `shared_by_topology` directory. This was something that I missed back porting to open source, but when doing it I realized that there was a bug in the code because the assignment could change and the directory would not be updated so I opted to just create it all the time instead, but I didn't update the param being passed in. If you want me to change it I am happy to.
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r213030742
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -125,6 +129,12 @@ public long getRemoteVersion(ClientBlobStore store) throws KeyNotFoundException,
@Override
public long fetchUnzipToTemp(ClientBlobStore store)
throws IOException, KeyNotFoundException, AuthorizationException {
+ synchronized (LocallyCachedTopologyBlob.class) {
+ if (!Files.exists(topologyBasicBlobsRootDir)) {
--- End diff --
If there are multiple supervisors running on a single host they all have to have different directories or a lot of really bad stuff will happen all throughout the code base.
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r212787423
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -125,6 +129,12 @@ public long getRemoteVersion(ClientBlobStore store) throws KeyNotFoundException,
@Override
public long fetchUnzipToTemp(ClientBlobStore store)
throws IOException, KeyNotFoundException, AuthorizationException {
+ synchronized (LocallyCachedTopologyBlob.class) {
+ if (!Files.exists(topologyBasicBlobsRootDir)) {
--- End diff --
An class lock may be not sufficient if we have multiple blob instance on one host( or multi supervisors cause one supervisor will have a blob instance), and there will be a write conflict， a host scope file lock would be better.
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r212838119
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -125,6 +129,12 @@ public long getRemoteVersion(ClientBlobStore store) throws KeyNotFoundException,
@Override
public long fetchUnzipToTemp(ClientBlobStore store)
throws IOException, KeyNotFoundException, AuthorizationException {
+ synchronized (LocallyCachedTopologyBlob.class) {
+ if (!Files.exists(topologyBasicBlobsRootDir)) {
--- End diff --
If my understanding is right, your point looks like only valid when there're multiple Supervisors running in a host with same base directory, and IMHO it is not the thing that we support. Are there other cases we need to handle with file lock?
---
[GitHub] storm issue #2812: STORM-3203: add back in the permission updates
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on the issue:
https://github.com/apache/storm/pull/2812
+1, LGTM
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r213125537
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -53,21 +54,24 @@
private final boolean isLocalMode;
private final Path topologyBasicBlobsRootDir;
private final AdvancedFSOps fsOps;
+ private final String owner;
private volatile long version = NOT_DOWNLOADED_VERSION;
private volatile long size = 0;
+
/**
* Create a new LocallyCachedBlob.
- *
- * @param topologyId the ID of the topology.
+ * @param topologyId the ID of the topology.
* @param type the type of the blob.
+ * @param assignment the assignment, mostly to know who the owner is.
*/
protected LocallyCachedTopologyBlob(final String topologyId, final boolean isLocalMode, final Map<String, Object> conf,
- final AdvancedFSOps fsOps, final TopologyBlobType type) throws IOException {
+ final AdvancedFSOps fsOps, final TopologyBlobType type, LocalAssignment assignment) throws IOException {
super(topologyId + " " + type.getFileName(), type.getKey(topologyId));
this.topologyId = topologyId;
this.type = type;
this.isLocalMode = isLocalMode;
this.fsOps = fsOps;
+ this.owner = assignment.get_owner();
topologyBasicBlobsRootDir = Paths.get(ConfigUtils.supervisorStormDistRoot(conf, topologyId));
--- End diff --
Yes please. Let's decouple unnecessary things.
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2812#discussion_r212787378
--- Diff: storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java ---
@@ -53,21 +54,24 @@
private final boolean isLocalMode;
private final Path topologyBasicBlobsRootDir;
private final AdvancedFSOps fsOps;
+ private final String owner;
private volatile long version = NOT_DOWNLOADED_VERSION;
private volatile long size = 0;
+
/**
* Create a new LocallyCachedBlob.
- *
- * @param topologyId the ID of the topology.
+ * @param topologyId the ID of the topology.
* @param type the type of the blob.
+ * @param assignment the assignment, mostly to know who the owner is.
*/
protected LocallyCachedTopologyBlob(final String topologyId, final boolean isLocalMode, final Map<String, Object> conf,
- final AdvancedFSOps fsOps, final TopologyBlobType type) throws IOException {
+ final AdvancedFSOps fsOps, final TopologyBlobType type, LocalAssignment assignment) throws IOException {
super(topologyId + " " + type.getFileName(), type.getKey(topologyId));
this.topologyId = topologyId;
this.type = type;
this.isLocalMode = isLocalMode;
this.fsOps = fsOps;
+ this.owner = assignment.get_owner();
topologyBasicBlobsRootDir = Paths.get(ConfigUtils.supervisorStormDistRoot(conf, topologyId));
--- End diff --
I we passed `LocalAssignment` just to fetch the owner, why not just to pass in the owner directly, just curious.
---
[GitHub] storm issue #2812: STORM-3203: add back in the permission updates
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2812
@danny0405 @HeartSaVioR
I think I answered your questions.
---
[GitHub] storm pull request #2812: STORM-3203: add back in the permission updates
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2812
---