Allow custom geoip databases to be updated at runtime.

[martijnvg]

Custom geoip databases can be provided via the config/ingest-geoip directory,
which are loaded at node startup time. This change adds the functionality
that reloads custom databases at runtime.

• Extracted logic that loads builtin and custom databases into a separate class.
• The builtin databases serve always as a fallback for when no database can be found in config dir.
• Added qa module that tests reloading of databases at runtime.

[elasticmachine]

Pinging @elastic/es-core-features (Team:Core/Features)

[probakowski]

I don't really understand idea in LocalDatabases around configDatabases access, left some comments

[probakowski]

Is this synchronized in any way? Is it possible that we will add new key during invalidation of old ones?

[martijnvg]

Good catch. This does need some synchronization, otherwise there may be a chance that keys are added for a database to be deleted.

[martijnvg]

I added reference counting to DatabaseReaderLazyLoader, so that keys for the old databases are only removed immediately after the update if there are no usages from geoip processor instances or when the last geoip processor has completed doing geo enrichment.

This also addresses another issue; when a geoip processor gets a DatabaseReaderLazyLoader instance and then a database update occurs for the same database then the maxmind db reader (which is wrapped by DatabaseReaderLazyLoader) may fail with an 'com.maxmind.db.ClosedDatabaseException: The MaxMind DB has been closed.' error. The reference counting avoids closing the reader if it is being used.

[probakowski]

I think we should use ConcurrentHashMap for this.configDatabases, then we don't have to do copying and it should be safer for multi-threading.
Currently there's no guarantee that this.configDatabases in line 109 is the same as in 127 - get-and-update access should always include while loop and atomic swap (or something like Atomic<Something>.updateAndGet())

[martijnvg]

So I picked this approach because of the following reason:

• I think this method will not be invoked concurrently if there are multiple file updates. At least this is how understand the ResourceFileWatcher. The synchronizations happens here: ResourceWatcherService line 165, the run() method is synchronized. This then delegates to FileWatcher.FileObserver#checkAndNotify() and checks whether the files to be monitored have changed and then invoke this GeoipDirectoryListener for each change.
• I opted for the copy on write update mechanism here, because that is more efficient in cases where incidental changes are made and the resource is mainly read. This is a pattern we use elsewhere in the code base, where there is a resources, that is mainly read and rarely updated.

I'm okay with changing the configDatabases field to use ConcurrentHashMap, then we don't need the update logic here anymore, which makes this class simpler.

[probakowski]

I'd argue that ConcurrentHashMap will be actually faster here - get has no penalty at all and put/remove has to do less than copying whole map (with no contention it's just 1)find slot 2)add to list, done). And it gives you safety if we ever refactor updateDatabase to be invoked concurrently

[martijnvg]

👍 let's then go with ConcurrentHashMap

[probakowski]

Any reason for creating copy here? Collections.unmodifiableMap(databases) should suffice I think

[martijnvg]

It only needs to be immutable, so like you suggest Collections.unmodifiableMap(...) suffices.

[probakowski]

Any reason for creating copy here? Collections.unmodifiableMap(databases) should suffice I think

[martijnvg]

I will change this, see the previous comment.

[probakowski]

Suggested change

	DatabaseReaderLazyLoader database = configDatabases.get(name);
	if (database != null) {
	return database;
	} else {
	return defaultDatabases.get(name);
	return configDatabases.getOrDefault(name, defaultDatabases.get(name));

[probakowski]

One problem found + 2 small nits

[probakowski]

I think there's still race here that can lead to loader not being closed, consider this:
t1 calls close() and line 212 is executed
t2 calls preLookup()
t1 checks line 213 and doesn't close the loader (currentUsages == 1)
t2 checks line 95 LocalDatabases and sees loader "closed"
t2 never calls postLookup() and loader is never really closed

We should either relay on single atomic variable for both checks (Integer.MIN_VALUE as close state for example) or call postLookup() in else clause in LocalDatabases

[probakowski]

Something like that should work but it's not tested (it uses negative values as indicator of close state, you have to check result of preLookup instead of isClosed)

boolean preLookup() {
    return currentUsages.updateAndGet(current -> current < 0 ? current : current + 1) > 0;
}

void postLookup() throws IOException {
    if (currentUsages.updateAndGet(current -> current > 0 ? current - 1 : current + 1) == -1) {
        doClose();
    }
}

@Override
public void close() throws IOException {
    if (currentUsages.updateAndGet(u -> -1 - u) == -1) {
        doClose();
    }
}

[martijnvg]

Good point, my forgot to add an else clause to LocalDatabases#getDatabase(...) method.
I prefer your approach (only one atomic integer that does all the accounting) over this approach.
Also stress tested it and your approach works correctly.

[probakowski]

no need for explicit initialization I think

[probakowski]

maybe return null here to make it explicit

[probakowski]

LGTM, thanks @martijnvg !