Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-4777 get schema by id even if not latest #2405

Closed
wants to merge 1 commit into from

Conversation

adfel70
Copy link

@adfel70 adfel70 commented Jan 15, 2018

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?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
  • If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
  • If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

  • 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.

@markap14
Copy link
Contributor

@adfel70 thanks for the contribution! I'm a bit concerned about the ramifications of this change as-is, though. If I'm understanding the code change, it appears to be removing all caching from the controller service. I don't think that's what we want to do, as network problems or having the Confluent Schema Registry go down would cause the dataflow to stop. I think if we want to avoid the caching mechanism, then we would need to expose a property to allow the user to choose whether or not they want to do so - or how long they want to cache the schema.

@adfel70
Copy link
Author

adfel70 commented Jan 17, 2018

@markap14,
The schema registry still has a cache in here.
There were previously 2 caches, that was the reason i removed the ones in the rest client.
Instead the code runs and gets the schema at the particular id, instead of the latest, since it created a bug in our nifi workflow.

@adfel70
Copy link
Author

adfel70 commented Jan 21, 2018

Hey could anyone else help me with this?
if it is compulsory i can return the second cache

@asfgit asfgit closed this in 1091093 Mar 19, 2018
@markap14
Copy link
Contributor

Hey @adfel70 I think I was looking at this PR the wrong way. I didn't notice the addition of the POST request, which is very vital. Because of that, I thought this was making a very large number of requests, but after reviewing the confluent API docs and reviewing the code again it all makes sense. The caching in this layer is different than the caching at the higher layer but was only really necessary for the routine that was there previously, before the POST request. I think your solution handles this case much better than the previous solution. And more importantly your solution appears to be correct :) I was able to test locally and see that if i changed the schema reference to an older version of an existing schema that things work correctly with this PR and did not without the PR. So I've merged this to master. Thanks for the fix and for the patience in getting it all sorted out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants