Skip to content

NIFI-10693 Remove direct proxy configuration properties from PutBigQuery#6580

Closed
bejancsaba wants to merge 3 commits intoapache:mainfrom
bejancsaba:NIFI-10693-Remove-Direct-Proxy-Conf-From-BigQuery
Closed

NIFI-10693 Remove direct proxy configuration properties from PutBigQuery#6580
bejancsaba wants to merge 3 commits intoapache:mainfrom
bejancsaba:NIFI-10693-Remove-Direct-Proxy-Conf-From-BigQuery

Conversation

@bejancsaba
Copy link
Contributor

Summary

NIFI-10693

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 8
    • JDK 11
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@bejancsaba bejancsaba requested a review from turcsanyip October 25, 2022 19:13
Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement @bejancsaba.

How will this impact existing deployments using this Processor?

The InvokeHTTP Processor supports both direct Proxy configuration and Proxy Configuration Service properties for compatibility. Recent changes added deprecation logging warnings for use of the direct properties, so it seems like that might be necessary for now to avoid breaking flows that use the direct properties.

@turcsanyip
Copy link
Contributor

@exceptionfactory Newly added processors should only support the Proxy (or SSL) controller service and not the direct configuration properties.
PutBigQuery was just added in 1.18.0 so in my opinion it may be acceptable to remove those properties with mentioning it in the Migration Guidance.

@exceptionfactory
Copy link
Contributor

Thanks for the background on release timing @turcsanyip.

I agree that new Processors should only use the Proxy Configuration Service. Since this Processor was just released in 1.18.0, and because the individual Proxy properties are optional, it seems reasonable to move forward with removing the individual properties and adding a note to Migration Guidance for subsequent release versions.

It looks like there are automated test failures related to the property changes that need to be addressed, but the general plan sounds good in light of that background.

@turcsanyip
Copy link
Contributor

Thanks for your answer @exceptionfactory!

Actually, while I was testing the processor I noticed that the proxy is not used at all (configured either via the legacy properties or the controller service). So at least we will not break anything working.

It seems it is not enough to add the proxy config for the HTTP transport, but we need to add it for GRPC too. It can be done via BigQueryWriteSettings.Builder.setTransportChannelProvider() but the config is not straightforward.

@bejancsaba I think we can either fix the proxy here or re-scope this PR, remove the proxy config at all and add a follow-up jira for fixing it.

@bejancsaba
Copy link
Contributor Author

@bejancsaba I think we can either fix the proxy here or re-scope this PR, remove the proxy config at all and add a follow-up jira for fixing it.

Removed the proxy configuration for now and created a dedicated Jira for the Proxy work: https://issues.apache.org/jira/browse/NIFI-10707

Copy link
Contributor

@turcsanyip turcsanyip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bejancsaba and @exceptionfactory!

+1 LGTM
Mergng to main.

@asfgit asfgit closed this in ac7306f Oct 27, 2022
priyanka-28 pushed a commit to priyanka-28/nifi that referenced this pull request Nov 16, 2022
This closes apache#6580.

Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
lizhizhou pushed a commit to lizhizhou/nifi that referenced this pull request Jan 2, 2023
This closes apache#6580.

Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants