You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@streams.apache.org by robdouglas <gi...@git.apache.org> on 2014/09/09 22:33:36 UTC
[GitHub] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
GitHub user robdouglas opened a pull request:
https://github.com/apache/incubator-streams/pull/80
STREAMS-166 | Modified the S3PeristReader to check for object summaries ...
...in the listing response. If there are any object summaries then we can assume that the path is pointing to a directory and we need to iterate through all files that it contains.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/robdouglas/incubator-streams STREAMS-166
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-streams/pull/80.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 #80
----
commit 8fb07eb2f3ebfae42f612ad169c06115914d8a4f
Author: Robert Douglas <rd...@w2odigital.com>
Date: 2014-09-09T20:32:50Z
STREAMS-166 | Modified the S3PeristReader to check for object summaries in the listing response. If there are any object summaries then we can assume that the path is pointing to a directory and we need to iterate through all files that it contains.
----
---
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] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on the pull request:
https://github.com/apache/incubator-streams/pull/80#issuecomment-56387888
Yeah you are right, it turns out to be the same $$$ [1], and this reader doesn't have too many configuration parameters ;) [2] but this is just my 2c.
I think the changes are cool besides that :+1:
[1] http://aws.amazon.com/s3/pricing/
[2]https://github.com/apache/incubator-streams/blob/master/streams-contrib/streams-amazon-aws/streams-persist-s3/src/main/jsonschema/org/apache/streams/s3/S3ReaderConfiguration.json
---
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] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on the pull request:
https://github.com/apache/incubator-streams/pull/80#issuecomment-55094096
Hi Robert,
This looks great, but wouldn't it be better if you put the 500 value used for configuring the S3 reader as a constant? And maybe adding a simple test for this feature? I mean reading from a directory or from a single object, or at least opening a JIRA issue for 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] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
Posted by rbnks <gi...@git.apache.org>.
Github user rbnks commented on the pull request:
https://github.com/apache/incubator-streams/pull/80#issuecomment-56197255
No tests where created or modified with this pull request, please explain.
---
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] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-streams/pull/80
---
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] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
Posted by smashew <gi...@git.apache.org>.
Github user smashew commented on the pull request:
https://github.com/apache/incubator-streams/pull/80#issuecomment-56286723
It is per listing you get back... I think. It turns out to be the same $$$ either way
Sent from my iPhone
> On Sep 20, 2014, at 5:17 PM, Renato Marroquin <no...@github.com> wrote:
>
> @smashew I see, this totally makes sense then. But then it the cost is 1cent per 1000, then wouldn't it be better to use 1000?
> I do understand your point, having too many configurations make it more difficult, but I don't think that having to recompile the code for a specific use case makes it less difficult. IMHO we should use a default value like 500 and if the user wants to change it, then he should be able to do it using the config files.
>
> —
> Reply to this email directly or view it on GitHub.
---
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] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
Posted by smashew <gi...@git.apache.org>.
Github user smashew commented on the pull request:
https://github.com/apache/incubator-streams/pull/80#issuecomment-56198437
@renato2099 With the way the reader works, there is a price for an object listing... (1 cent per 1,000), seeing as the stream is exhausting the entire directory instead of reading just a portion, the constant of 50 || 100 is inconsequential. bumping to 500 will limit the # of round trip calls however.
Agree, that it should be a setting, but presumably, one does run the risk of having soo many settings that it is too difficult to use. :o)
---
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] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
Posted by sieverssj <gi...@git.apache.org>.
Github user sieverssj commented on the pull request:
https://github.com/apache/incubator-streams/pull/80#issuecomment-55034509
:+1:.
I accidentally started this review on an internal fork. https://github.com/w2ogroup/incubator-streams/pull/52
---
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] incubator-streams pull request: STREAMS-166 | Modified the S3Peris...
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on the pull request:
https://github.com/apache/incubator-streams/pull/80#issuecomment-56282179
@smashew I see, this totally makes sense then. But then it the cost is 1cent per 1000, then wouldn't it be better to use 1000?
I do understand your point, having too many configurations make it more difficult, but I don't think that having to recompile the code for a specific use case makes it less difficult. IMHO we should use a default value like 500 and if the user wants to change it, then he should be able to do it using the config files.
---
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.
---