You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by brosander <gi...@git.apache.org> on 2017/01/25 18:01:49 UTC
[GitHub] nifi pull request #1446: NIFI-3389 - Using long string type for attribute na...
GitHub user brosander opened a pull request:
https://github.com/apache/nifi/pull/1446
NIFI-3389 - Using long string type for attribute name and value in Fl\u2026
\u2026owFileSchema
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:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [X] 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.
- [X] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [X] Is your initial contribution a single, squashed commit?
### For code changes:
- [X] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [X] Have you written or updated unit tests to verify your changes?
- [X] - 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)?
- [X] - N/A - If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [X] - N/A - If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [X] - 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:
- [X] - 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/brosander/nifi NIFI-3389
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1446.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 #1446
----
----
---
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 #1446: NIFI-3389 - Using long string type for attribute name and ...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1446
OK looks good, verified behavior, good is good. +1 merged to master. Thanks for fixing this!
---
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 #1446: NIFI-3389 - Using long string type for attribute na...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1446#discussion_r101047085
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/schema/RepositoryRecordSchema.java ---
@@ -91,4 +97,42 @@
final UnionRecordField repoUpdateField = new UnionRecordField(REPOSITORY_RECORD_UPDATE_V1, Repetition.EXACTLY_ONE, createOrUpdate, delete, swapOut, swapIn);
REPOSITORY_RECORD_SCHEMA_V1 = new RecordSchema(Collections.singletonList(repoUpdateField));
}
+
+ static {
--- End diff --
I believe the only difference between this static block and the other is which FlowFileSchema should be used (FLOWFILE_SCHEMA_V1 vs. FLOWFILE_SCHEMA_V2), correct? This is quite a lot of code to duplicate. Can we instead just have a
`private static RecordSchema createSchema(RecordSchema flowFileSchema)`
type of method, and have it just use the given FlowFileSchema? I believe this would eliminate all of the duplication.
---
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 #1446: NIFI-3389 - Using long string type for attribute name and ...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on the issue:
https://github.com/apache/nifi/pull/1446
@markap14 I was thinking this might be a time when verbosity makes it less likely that someone would inadvertently change old behavior
I can pull the logic out into methods but anyone wanting to further change the v2 schema would have to be careful not to also impact the v1 schema.
---
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 #1446: NIFI-3389 - Using long string type for attribute na...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1446#discussion_r101047529
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/swap/SwapSchema.java ---
@@ -76,4 +78,32 @@
fullSchemaFields.add(new ComplexRecordField(FLOWFILE_CONTENTS, Repetition.ZERO_OR_MORE, FlowFileSchema.FLOWFILE_SCHEMA_V1.getFields()));
FULL_SWAP_FILE_SCHEMA_V1 = new RecordSchema(fullSchemaFields);
}
+
+ static {
--- End diff --
Again, as above, we should have a method that builds the RecordSchema.
---
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 #1446: NIFI-3389 - Using long string type for attribute name and ...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on the issue:
https://github.com/apache/nifi/pull/1446
Rebased PR, updated tests to account for https://github.com/apache/nifi/pull/1469
---
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 #1446: NIFI-3389 - Using long string type for attribute na...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/1446
---
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 #1446: NIFI-3389 - Using long string type for attribute name and ...
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the issue:
https://github.com/apache/nifi/pull/1446
I built and ran with this PR on top of the PR #1475, and the WARN messages produced by huge flowfile attributes no longer occur. Test dataflow works fine. I also upgraded a NiFi that was using a V1 Schema to a NiFi with this PR included, and it could read the repositories and continue to work with huge flowfile attributes.
I am +1 due to this PR achieving the desired result. I did not code review it, though. I would prefer other eyes on this.
---
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 #1446: NIFI-3389 - Using long string type for attribute name and ...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1446
@brosander I can buy that logic. It may be better to provide the ability to look at the code and see "This is exactly what v1 looked like. This is exactly what v2 looked like." as opposed to simplifying the code base. As the code base changes over time, it may get brutal to maintain it all within a class, but we can break it out at that point if necessary.
---
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 #1446: NIFI-3389 - Using long string type for attribute na...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1446#discussion_r101047296
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/schema/FlowFileSchema.java ---
@@ -64,4 +65,23 @@
FLOWFILE_SCHEMA_V1 = new RecordSchema(flowFileFields);
}
+
+ static {
--- End diff --
I think the same comment as below applies here as well - we can have a private static method that takes an argument of type FieldType and builds this RecordSchema, rather than duplicating all of this code.
---
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.
---