[GitHub] flink issue #2288: Feature/s3 a fix

[cresny]

Github user cresny commented on the issue:

https://github.com/apache/flink/pull/2288
  
@uce I finally got around to fixing this. Regarding your above comments:

- I think we got the "flattening" concern backwards. What I meant and 
wanted to avoid was to move all files into single directory. The calling code 
asks to simply upload the lib dir, and I think it should be copied structurally 
intact. I think I saw on the user list a complaint that properties files were 
not moved -- this should fix that. Or am I missing some other concern?

- The new commit 26c8511701d6b852a3c6f8b306f4b5da8e7b8479 now only modifies 
flink-yarn/src/main/java/org/apache/flink/yarn/Utils.java

- I kept the check for file:// scheme for now just because I think it's 
safer to check than assume it's set down the call stack but I can hunt down and 
change those calls if you think it's best. 

Since this PR is pretty tortured, maybe I should create a new one with just 
the above commit?




---
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 issue #2288: Feature/s3 a fix

[uce]

Github user uce commented on the issue:

https://github.com/apache/flink/pull/2288
  
Exactly @cresny. If you have time, would be nice to get this in.


---
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 issue #2288: Feature/s3 a fix

[cresny]

Github user cresny commented on the issue:

https://github.com/apache/flink/pull/2288
  
I was hoping that when the 1.2-SNAPSHOT dust cleared it would no longer be
necessary.  At this point it seems it's only necessary for staging a YARN
session for an S3A configured FileSystem, correct? If so I can wrap it up
in the next few days.

On Wed, Oct 12, 2016 at 2:25 PM, Stephan Ewen 
wrote:

> @cresny  Do you want to continue on this? I
> think it is a good fix.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or 
mute
> the thread
> 

> .
>



---
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 issue #2288: Feature/s3 a fix

[StephanEwen]

Github user StephanEwen commented on the issue:

https://github.com/apache/flink/pull/2288
  
@cresny Do you want to continue on this? I think it is a good fix.


---
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 issue #2288: Feature/s3 a fix

[uce]

Github user uce commented on the issue:

https://github.com/apache/flink/pull/2288
  
Hey Cliff! I looked over this again and I think we have to address some 
things:
- The problem you mentioned on the mailing list with respect to to 
flattening is actually something we should not ignore. Can't we create the 
directory manually and then copy the files there instead of flattening?
- With #2345 the RocksDB part will be removed and we only use 
`copyFromLocal` in YARN for JAR copies. Therefore I would move the utility 
method to `flink-yarn/src/main/java/org/apache/flink/yarn/Utils.java` and only 
use it in YARN (ignore the RocksDB part).
- The check for the `file://` scheme is unrelated and we should address in 
a separate issue (remove `checkFileScheme` and `checkInitialDirectory`). This 
might point to a bug in some other place actually.

Feel free to ping me if you don't have time to address this and I can also 
do it. Sorry for the back and forth.


---
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 issue #2288: Feature/s3 a fix

[cresny]

Github user cresny commented on the issue:

https://github.com/apache/flink/pull/2288
  
refactored commit here: 
https://github.com/apache/flink/pull/2288/commits/8505e9c77f5ed79542af259f007be2c3a29edbc5

self-tested working in upstream master


---
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 issue #2288: Feature/s3 a fix

[uce]

Github user uce commented on the issue:

https://github.com/apache/flink/pull/2288
  
Thank you! :+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2288: Feature/s3 a fix

[cresny]

Github user cresny commented on the issue:

https://github.com/apache/flink/pull/2288
  
I just returned from vacation so I have some catching up to do, but I'll
take care of this today or tomorrow.

On Aug 15, 2016 6:22 AM, "Ufuk Celebi"  wrote:

> Thank you for contributing. All in all, this looks good and is an
> important fix. We would have to address the inline comments I've raised
> before merging it though. Do you have time to address them?
>
> For the commits, I would squash all to a single commit via git rebase -i
> and add the issue tag and a description of what the problem is and how we
> solve it.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> , or 
mute
> the thread
> 

> .
>



---
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 issue #2288: Feature/s3 a fix

[uce]

Github user uce commented on the issue:

https://github.com/apache/flink/pull/2288
  
Thank you for contributing. All in all, this looks good and is an important 
fix. We would have to address the inline comments I've raised before merging it 
though. Do you have time to address them?

For the commits, I would squash all to a single commit via `git rebase -i` 
and add the issue tag and a description of what the problem is and how we solve 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2288: Feature/s3 a fix

[uce]

Github user uce commented on the issue:

https://github.com/apache/flink/pull/2288
  
The associated issue for this can be found here: 
https://issues.apache.org/jira/browse/FLINK-4228.

+1 to the points Stephan raised in his earlier comment.




---
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 issue #2288: Feature/s3 a fix

[StephanEwen]

Github user StephanEwen commented on the issue:

https://github.com/apache/flink/pull/2288
  
Thank you for this contribution.
Can you help us a bit so we can review this?
  - Remove the template text for the Pull Request
  - Tag the pull request with the JIRA issue it addresses
  - Describe briefly what was the original bug
  - Describe briefly what is the solution implemented here.

That allows us to check the code and better see if we can think of any 
possible side effects of this 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---