[GitHub] flink pull request #3896: [FLINK-6370] [webUI] Handle races for single file ...

[zentol]

Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/3896#discussion_r116500253
  
--- Diff: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/files/StaticFileServerHandler.java
 ---
@@ -234,8 +237,17 @@ private void respondAsLeader(ChannelHandlerContext 
ctx, HttpRequest request, Str
if 
(!rootURI.relativize(requestedURI).equals(requestedURI)) {

logger.debug("Loading missing file from classloader: {}", requestPath);
// ensure that 
directory to file exists.
-   
file.getParentFile().mkdirs();
-   
Files.copy(resourceStream, file.toPath());
+   if 
(!file.getParentFile().mkdirs()) {
+   throw 
new IOException("Could not create directories for file " + file);
+   }
+   synchronized 
(COPY_LOCK) {
--- End diff --

There is nothing stopping us from changing to eager loading later on, so I 
opted for the fastest, yet still reasonable, fix for the problem.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3896: [FLINK-6370] [webUI] Handle races for single file ...

[zentol]

Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/3896#discussion_r116500027
  
--- Diff: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/files/StaticFileServerHandler.java
 ---
@@ -234,8 +237,17 @@ private void respondAsLeader(ChannelHandlerContext 
ctx, HttpRequest request, Str
if 
(!rootURI.relativize(requestedURI).equals(requestedURI)) {

logger.debug("Loading missing file from classloader: {}", requestPath);
// ensure that 
directory to file exists.
-   
file.getParentFile().mkdirs();
-   
Files.copy(resourceStream, file.toPath());
+   if 
(!file.getParentFile().mkdirs()) {
+   throw 
new IOException("Could not create directories for file " + file);
+   }
+   synchronized 
(COPY_LOCK) {
--- End diff --

Because this was easy to write, is easy to review, does not change any 
behavior and isn't particularly intrusive.

I agree that we should have a discussion as to whether we should load the 
files lazily or not, but not now when the next release is coming up and 
everyone is scrambling to fix the most issues in as little time as possible


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3896: [FLINK-6370] [webUI] Handle races for single file ...

[dernasherbrezon]

Github user dernasherbrezon commented on a diff in the pull request:

https://github.com/apache/flink/pull/3896#discussion_r116474287
  
--- Diff: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/files/StaticFileServerHandler.java
 ---
@@ -234,8 +237,17 @@ private void respondAsLeader(ChannelHandlerContext 
ctx, HttpRequest request, Str
if 
(!rootURI.relativize(requestedURI).equals(requestedURI)) {

logger.debug("Loading missing file from classloader: {}", requestPath);
// ensure that 
directory to file exists.
-   
file.getParentFile().mkdirs();
-   
Files.copy(resourceStream, file.toPath());
+   if 
(!file.getParentFile().mkdirs()) {
+   throw 
new IOException("Could not create directories for file " + file);
+   }
+   synchronized 
(COPY_LOCK) {
--- End diff --

Looks like workaround. Why not simply extract all static resources on 
startup?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3896: [FLINK-6370] [webUI] Handle races for single file ...

[zentol]

Github user zentol closed the pull request at:

https://github.com/apache/flink/pull/3896


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3896: [FLINK-6370] [webUI] Handle races for single file ...

[zentol]

GitHub user zentol opened a pull request:

https://github.com/apache/flink/pull/3896

[FLINK-6370] [webUI] Handle races for single file in FileServerHandler

This PR prevents a race between multiple requests trying to create the same 
file, which previously would cause one request to fail.

We can't just ignore the `FileAlreadyExistsException` since there's no 
guarantee that the file copying is complete. To prevent this scenario the 
copying is now done in a `synchronized` block, along with ignoring the 
`FileAlreadyExistsExceptions`.

Due to the small size of files loaded from the class-loader this should 
have no impact on the responsiveness of the webUI.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/zentol/flink 6370_web_file

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/3896.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 #3896


commit 4ebee58fa8c39d0aa6d68d1470ea3a850e92b954
Author: zentol 
Date:   2017-05-14T19:57:16Z

[FLINK-6370] [webUI] Handle races for single file in FileServerHandler




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---