-
Notifications
You must be signed in to change notification settings - Fork 215
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
Update geoip config and use extensions #3975
Update geoip config and use extensions #3975
Conversation
Signed-off-by: Asif Sohail Mohammed <[email protected]>
Signed-off-by: Asif Sohail Mohammed <[email protected]>
Signed-off-by: Asif Sohail Mohammed <[email protected]>
Signed-off-by: Asif Sohail Mohammed <[email protected]>
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.
Nice, this is looking good.
*/ | ||
public GeoIPProcessorService(GeoIPProcessorConfig geoIPProcessorConfig, String tempPath) { | ||
public GeoIPProcessorService(final GeoIpServiceConfig geoIpServiceConfig) { |
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.
I think this is going to be provided by the extension in the end, right?
It would be good to move this into the .extension
package. That could happen in a follow-on PR.
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.
Yes, it's provided by extension supplier. I will move it to right package.
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.
QUES: do we know if this will potentially be used in other pipeline plugins? Need to catch up on justification why we put it in extensions
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.
I don't think it will be used by other plugins. But the extension will allow Data Prepper to run a single GeoIP "service" which any number of processors can share.
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.
By "number of processors" are we expecting future processor plugin to depend upon this? It would be good to have examples but not blocking this PR.
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.
I mean any number of geoip
processors.
pipeline-a:
processor:
- geoip:
- geoip:
pipeline-b:
processor:
- geoip:
All of these will share the same underlying objects and resources from Maxmind.
@@ -42,13 +41,13 @@ public HttpDBDownloadService(String prefixDir) { | |||
* Initialisation of Download through Url | |||
* @param urlList urlList | |||
*/ | |||
public void initiateDownload(List<DatabasePathURLConfig> urlList) throws Exception { | |||
public void initiateDownload(List<String> urlList) { |
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.
This entire package (databasedownload
) could probably be put under the .extension
package as well - .extension.databasedownload
.
Again, this could be a follow-on PR.
Description
This PR updates geoip processor configuration and also uses extensions created in #3944
Old config:
New pipeline config
Data prepper config
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.