You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by vs186031 <gi...@git.apache.org> on 2015/10/23 20:46:45 UTC
[GitHub] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
GitHub user vs186031 opened a pull request:
https://github.com/apache/nifi/pull/107
NiFI-1025: Imported aws-sdk:1.10.27 and joda-time: 2.8.2
NiFI-1025: Imported aws-sdk:1.10.27 and joda-time: 2.8.2 to resolve AWS issues with Java 1.8 update 60.
Re-did the tests to make them :
a_ clean up after themselves.
b_ fail if initial setup conditions are not met.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vs186031/nifi nifi-1025
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/107.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 #107
----
commit 4885905bce849cfed0507d8358c80550e6f9461f
Author: Venkatesh Sellappa <vs...@outlook.com>
Date: 2015-10-23T18:40:52Z
NiFI-1025: Imported aws-sdk:1.10.27 and joda-time: 2.8.2 to resolve AWS issues with Java 1.8 update 60
----
---
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] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on a diff in the pull request:
https://github.com/apache/nifi/pull/107#discussion_r43836211
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/s3/TestPutS3Object.java ---
@@ -87,16 +93,29 @@ public void testStorageClass() throws IOException {
@Test
public void testPermissions() throws IOException {
final TestRunner runner = TestRunners.newTestRunner(new PutS3Object());
- runner.setProperty(PutS3Object.BUCKET, "test-bucket-00000000-0000-0000-0000-123456789012");
+
runner.setProperty(PutS3Object.CREDENTAILS_FILE, CREDENTIALS_FILE);
- runner.setProperty(PutS3Object.FULL_CONTROL_USER_LIST, "28545acd76c35c7e91f8409b95fd1aa0c0914bfa1ac60975d9f48bc3c5e090b5");
+ runner.setProperty(PutS3Object.BUCKET, BUCKET_NAME);
+ runner.setProperty(PutS3Object.FULL_CONTROL_USER_LIST,"28545acd76c35c7e91f8409b95fd1aa0c0914bfa1ac60975d9f48bc3c5e090b5");
+ runner.setProperty(PutS3Object.REGION, REGION);
final Map<String, String> attrs = new HashMap<>();
attrs.put("filename", "folder/4.txt");
- runner.enqueue(Paths.get("src/test/resources/hello.txt"), attrs);
+ runner.enqueue(getResourcePath(RESOURCE_NAME), attrs);
+
runner.run();
runner.assertAllFlowFilesTransferred(PutS3Object.REL_SUCCESS, 1);
}
-}
+ @Test
+ /**
+ * Run this test to check what version of Joda-time is actually being linked.
+ *
+ * @see: https://issues.apache.org/jira/browse/NIFI-1025
+ * @see: https://github.com/aws/aws-sdk-java/issues/444
+ */
+ public void testJodaTime() {
+ System.out.println(new DateTime().getClass().getProtectionDomain().getCodeSource());
--- End diff --
This doesn't appear to test for something - just a dump to the screen
---
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.
---
Re: [GitHub] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27
and joda-ti...
Posted by Tony Kurc <tr...@gmail.com>.
Understood! We appreciate the contribution!
On Sun, Nov 8, 2015 at 4:19 AM, Venkatesh Sellappa <vs...@outlook.com>
wrote:
> @trkurc: " We're going to try to get this into 0.4.0 release of NiFi - I
> can
> make the changes, or if you have a patch in progress, I'll work off of that
> "
>
> - I have been travelling and by the time i looked at this , the patch was
> in.
>
> Thanks for the help Tony.
>
>
>
> --
> View this message in context:
> http://apache-nifi-developer-list.39713.n7.nabble.com/GitHub-nifi-pull-request-NiFI-1025-Imported-aws-sdk-1-10-27-and-joda-ti-tp3291p4132.html
> Sent from the Apache NiFi Developer List mailing list archive at
> Nabble.com.
>
Re: [GitHub] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27
and joda-ti...
Posted by Venkatesh Sellappa <vs...@outlook.com>.
@trkurc: " We're going to try to get this into 0.4.0 release of NiFi - I can
make the changes, or if you have a patch in progress, I'll work off of that
"
- I have been travelling and by the time i looked at this , the patch was
in.
Thanks for the help Tony.
--
View this message in context: http://apache-nifi-developer-list.39713.n7.nabble.com/GitHub-nifi-pull-request-NiFI-1025-Imported-aws-sdk-1-10-27-and-joda-ti-tp3291p4132.html
Sent from the Apache NiFi Developer List mailing list archive at Nabble.com.
[GitHub] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/107
---
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] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the pull request:
https://github.com/apache/nifi/pull/107#issuecomment-153924808
A couple of comments:
I recommend changing S3Setup to something like AbstractS3Test and making it an abstract class (if for nothing else, because it doesn't contain test code and is the base class for the other tests)
What this method did was a bit misleading. I expected that the parameter was the filename. I'd rather see it with two parameters, key and filename, to make it a bit more general purpose. If these tests are extended with multiple files, having that second parameter would be helpful. Also, should it throw an AmazonS3Exception?
```
protected void uploadTestFile(String key) throws IOException {
```
In the following code, rather than printing to System.err, would Asserting make more sense? Or did you intend for the doesBucketExist be a catchall for these problems?
```
} catch (final AmazonS3Exception e) {
System.err.println("Can't create the key " + BUCKET_NAME + ":" + e.toString());
} catch (final IOException e) {
System.err.println(CREDENTIALS_FILE + " doesn't exist.");
}
if (!client.doesBucketExist(BUCKET_NAME)) {
Assert.fail("Setup incomplete, tests will fail");
}
```
This one confused me a bit in TestDeleteS3Object.testSimpleDelete. I doesn't quite match what the old testSimpleDelete method did, which was upload with key of hello.txt, with file hello.txt. I think you're uploading with key "folder" and with file "/hello.txt". I reasonably believe this does the same thing, but it was hard to read
```
public void testSimpleDelete() throws IOException {
// Prepares for this test
uploadTestFile("folder");
final TestRunner runner = TestRunners.newTestRunner(new DeleteS3Object());
runner.setProperty(DeleteS3Object.CREDENTAILS_FILE, CREDENTIALS_FILE);
runner.setProperty(DeleteS3Object.REGION, REGION);
runner.setProperty(DeleteS3Object.BUCKET, BUCKET_NAME);
runner.setProperty(DeleteS3Object.KEY, "folder");
```
On a similar note I think TestDeleteS3Object.testSimpleDeleteFolder was intended to delete with a key with a slash in it, as it was in the original test
```
@Test
public void testDeleteFolder() throws IOException {
// Prepares for this test
uploadTestFile("folder");
```
I think that you may have propagated a bug from TestDeleteS3Object.testTryToDeleteNotExistingFile [1]. I think the original test that second setting of BUCKET was erroneous. I think this should be BUCKET_NAME instead of "no-such-a-key".
```
runner.setProperty(DeleteS3Object.BUCKET, "no-such-a-key");
```
Also, any thought to trying to exercise the Attribute Evaluation of the KEY in the tests? All the KEY properties were set explicitly.
[1] https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/s3/TestDeleteS3Object.java#L122
---
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] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the pull request:
https://github.com/apache/nifi/pull/107#issuecomment-154717438
@vs186031 - We're going to try to get this into 0.4.0 release of NiFi - I can make the changes, or if you have a patch in progress, I'll work off of that
---
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] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the pull request:
https://github.com/apache/nifi/pull/107#issuecomment-153553020
I'm looking through the code. I'm wondering if you considered trying to convert these unit tests which require network calls to use mock services. It dawned on me as I was checking this through that these changes are all in @Ignore's
---
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] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the pull request:
https://github.com/apache/nifi/pull/107#issuecomment-153925206
Also, as I mentioned inline in the commit, this really isn't a unit test as much as an informational log.
```
@Test
/**
* Run this test to check what version of Joda-time is actually being linked.
*
* @see: https://issues.apache.org/jira/browse/NIFI-1025
* @see: https://github.com/aws/aws-sdk-java/issues/444
*/
public void testJodaTime() {
System.out.println(new DateTime().getClass().getProtectionDomain().getCodeSource());
}
```
---
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] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by vs186031 <gi...@git.apache.org>.
Github user vs186031 commented on the pull request:
https://github.com/apache/nifi/pull/107#issuecomment-153591624
@trkurc : Indeed they are all actual Network calls and have been annotated with *@Ignore*. I suspect ideally we want both Mock Services and _real_ network calls, with the real services being more important.
---
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] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the pull request:
https://github.com/apache/nifi/pull/107#issuecomment-154760946
@vs186031 I've started making some of these changes
---
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] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the pull request:
https://github.com/apache/nifi/pull/107#issuecomment-153927816
In TestFetchS3Object.testSimpleGet(), the key, "folder/" is a bit confusing. I'm not intensely familiar with the S3 java API, but I'm not sure how this fits with their "folder" concept [1].
```
public void testSimpleGet() throws IOException {
String key = "folder/";
uploadTestFile(key);
```
Same issue with TestFetchS3Object.testTryToFetchNotExistingFile as TestDeleteS3Object.testTryToDeleteNotExistingFile. I think this tests for a non-existent bucket, not file.
[1] http://docs.aws.amazon.com/AmazonS3/latest/UG/FolderOperations.html
---
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.
---