-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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-11439 - correct provenance reporting parameter #7173
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the Provenance Transit URI construction @greyp9. The strategy makes sense, just recommend an approach based on URI instead of URL construction.
String transitUri; | ||
try { | ||
final URL url = new URL(storage.getOptions().getHost()); | ||
transitUri = String.format("%s://%s.%s/%s", url.getProtocol(), bucketName, url.getHost(), key); | ||
} catch (MalformedURLException e) { | ||
transitUri = e.getClass().getSimpleName(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using URI.create()
avoids the checked MalformedURLException
and because the Storage API URL must be valid for making the request, recommend the following approach:
String transitUri; | |
try { | |
final URL url = new URL(storage.getOptions().getHost()); | |
transitUri = String.format("%s://%s.%s/%s", url.getProtocol(), bucketName, url.getHost(), key); | |
} catch (MalformedURLException e) { | |
transitUri = e.getClass().getSimpleName(); | |
} | |
final URI storageApiUri = URI.create(storage.getOptions().getHost()); | |
final String transitUri = String.format("%s://%s.%s/%s", uri,getScheme(), bucketName, uri.getHost(), key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's better.
String transitUri; | ||
try { | ||
final URL url = new URL(storage.getOptions().getHost()); | ||
transitUri = String.format("%s://%s.%s/%s", url.getProtocol(), bucket, url.getHost(), key); | ||
} catch (MalformedURLException e) { | ||
transitUri = e.getClass().getSimpleName(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the implementation could be moved to a protected getTransitUri()
method in AbstractGCSProcessor
, or the changed could be implemented in both classes.
String transitUri; | |
try { | |
final URL url = new URL(storage.getOptions().getHost()); | |
transitUri = String.format("%s://%s.%s/%s", url.getProtocol(), bucket, url.getHost(), key); | |
} catch (MalformedURLException e) { | |
transitUri = e.getClass().getSimpleName(); | |
} | |
final URI storageApiUri = URI.create(storage.getOptions().getHost()); | |
final String transitUri = String.format("%s://%s.%s/%s", uri,getScheme(), bucketName, uri.getHost(), key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the adjustments and addressing the feedback @greyp9, the latest version looks good! +1 merging
In previous PR, missed adjustment of URL supplied to provenance reporting subsystem.
Summary
NIFI-11439
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation