You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by Wesley-Lawrence <gi...@git.apache.org> on 2017/07/23 15:43:59 UTC
[GitHub] nifi pull request #2034: NIFI-4215 NiFi can now parse an Avro schema of a re...
GitHub user Wesley-Lawrence opened a pull request:
https://github.com/apache/nifi/pull/2034
NIFI-4215 NiFi can now parse an Avro schema of a record that references an already defined record, including itself.
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [✓] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [✓] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [✓] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [✓] Is your initial contribution a single, squashed commit?
### For code changes:
- [✓] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [✓] Have you written or updated unit tests to verify your changes?
- [N/A] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [N/A] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [N/A] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [N/A] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [N/A] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Wesley-Lawrence/nifi NIFI-4215
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2034.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 #2034
----
commit b708f328b97aab2074f0a28c47be172e75990dd5
Author: Wesley-Lawrence <we...@gmail.com>
Date: 2017-07-23T15:04:01Z
NIFI-4215 NiFi can now parse an Avro schema of a record that references an already defined record, including itself.
----
---
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 issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2034
Reviewing...
---
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 issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2034
@Wesley-Lawrence I believe this should be:
````java
if() {
} else {
}
````
---
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 issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2034
@Wesley-Lawrence I think this is a good fix, your approach to the solution seems solid. Thanks for adding the unit tests for the recursive and mutually-referential cases. Would you please:
1. Fix the checkstyle issue and remove the change to the checkstyle definitions
1. Optionally change the foundSchemas/knownRecordTypes exception message
1. Squash and rebase on 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2034
@Wesley-Lawrence Thanks for putting this PR together. I am still reviewing - the only immediate feedback I can give is that I would prefer not to update the checkstyle definition in pom.xml as part of this fix for the AvroSchemaRegistry issue.
---
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 issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by Wesley-Lawrence <gi...@git.apache.org>.
Github user Wesley-Lawrence commented on the issue:
https://github.com/apache/nifi/pull/2034
@pvillard31 Yup, you're right, that solves the issue for me 😃.
I got so caught up in the second `RightCurly` definitons saying `}` should be alone, and other `if`s being that way, I didn't see the correct style for `if ... else`s.
I'll fix that line, re-add the original `RightCurly` definition back in, and push a new squashed commit.
Thanks again, both of you.
---
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 issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by Wesley-Lawrence <gi...@git.apache.org>.
Github user Wesley-Lawrence commented on the issue:
https://github.com/apache/nifi/pull/2034
Thanks for taking a look @jvwing!
I didn't want to change it, but I keep getting the following error with it;
```
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.15:check (check-style) on project nifi-avro-record-utils: You have 1 Checkstyle violation. -> [Help 1]
```
Earlier in the maven log;
```
[INFO] --- maven-checkstyle-plugin:2.15:check (check-style) @ nifi-avro-record-utils ---
[WARNING] src/main/java/org/apache/nifi/avro/AvroTypeUtil.java[275:17] (blocks) RightCurly: '}' should be on the same line.
```
Which references a `}` I added here;
```
273 if (knownRecordTypes.containsKey(schemaFullName)) {
274 return knownRecordTypes.get(schemaFullName);
275 --> }
276 else {
```
However, this is the style used everywhere in NiFi, and is the one defined by the `RightCurly` section below the one I removed.
I suspect it's something weird in my environment, but removing the default `RightCurly` definition fixed my issue, and it looks like it was just left over from some old migration, so I figured it could be safely removed.
Out of curiosity, if you run a contrib check, do you get the same error I do?
---
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 #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2034#discussion_r130225722
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-avro-record-utils/src/main/java/org/apache/nifi/avro/AvroTypeUtil.java ---
@@ -218,6 +218,15 @@ private static Schema nullable(final Schema schema) {
* @return a Data Type that corresponds to the given Avro Schema
*/
public static DataType determineDataType(final Schema avroSchema) {
+ return determineDataType(avroSchema, new HashMap<>());
+ }
+
+ public static DataType determineDataType(final Schema avroSchema, Map<String, DataType> knownRecordTypes) {
+
+ if (knownRecordTypes == null) {
+ throw new IllegalArgumentException("'foundSchemas' cannot be null.");
--- End diff --
Did you mean this to say "knownRecordTypes" rather than "foundSchemas"?
---
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 #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2034
---
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 issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by Wesley-Lawrence <gi...@git.apache.org>.
Github user Wesley-Lawrence commented on the issue:
https://github.com/apache/nifi/pull/2034
Alright, changes made, and contrib-check still passing for me.
Thanks for the review @jvwing!
---
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 issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2034
Thanks @pvillard31!
@Wesley-Lawrence Don't worry about squashing yet, we can do that as a final step.
---
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 issue #2034: NIFI-4215 Fixed stackoverflow error when NiFi tries to par...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2034
Looks good. Thanks again for fixing this, @Wesley-Lawrence.
---
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.
---