Make EnterpriseGeoIpDownloaderLicenseListener project aware #129992
Conversation
samxbr
added
:Data Management/Ingest Node
elasticsearchmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
| } | ||
| event.state().forEachProject(projectState -> { | ||
| ProjectId projectId = projectState.projectId(); | ||
| final boolean hasMetadata = event.state().metadata().getProject(projectId).custom(INGEST_GEOIP_CUSTOM_METADATA_TYPE) != null; |
There was a problem hiding this comment.
Nit:
| final boolean hasMetadata = event.state().metadata().getProject(projectId).custom(INGEST_GEOIP_CUSTOM_METADATA_TYPE) != null; | |
| final boolean hasMetadata = projectState.metadata().custom(INGEST_GEOIP_CUSTOM_METADATA_TYPE) != null; |
| @@ -137,6 +152,14 @@ private void ensureTaskStopped() { | |||
| } | |||
| } | |||
| ); | |||
| persistentTasksService.sendRemoveRequest(ENTERPRISE_GEOIP_DOWNLOADER, MasterNodeRequest.INFINITE_MASTER_NODE_TIMEOUT, listener); | |||
| persistentTasksService.sendRemoveRequest( | |||
There was a problem hiding this comment.
I think we want to make use of the new method you added, right?
| persistentTasksService.sendRemoveRequest( | |
| persistentTasksService.sendProjectRemoveRequest( | |
| projectId, |
| final boolean masterNodeChanged = Objects.equals( | ||
| event.state().nodes().getMasterNode(), | ||
| event.previousState().nodes().getMasterNode() | ||
| ) == false; |
There was a problem hiding this comment.
We can extract this to outside the loop.
| * if this cluster change event involved the modification of custom geoip metadata OR a master node change | ||
| */ | ||
| if (ingestGeoIpCustomMetaChangedInEvent || (masterNodeChanged && hasIngestGeoIpMetadata.getOrDefault(projectId, false))) { | ||
| maybeUpdateTaskState(projectState); |
There was a problem hiding this comment.
We could also pass event.state().nodes().isLocalNodeElectedMaster() as a parameter together with a project ID. That way we don't need to convert the projects into project states. What do you think?
| @FixForMultiProject | ||
| private final ProjectResolver projectResolver = TestProjectResolvers.DEFAULT_PROJECT_ONLY; |
There was a problem hiding this comment.
Are you going to fix this in a follow-up PR soon, or are you using the default project ID because licenseStateChanged() is not going to be made project-aware anytime soon? If it's the former, this is fine, but if it's the latter, I don't think we need a @FixForMultiProject; we can either add @NotMultiProjectCapable as well or just leave the annotation out and add a comment.
Although enterprise GeoIp is not currently available in serverless, this makes
EnterpriseGeoIpDownloaderLicenseListenermostly project aware.