Skip to content
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

Support remote zilla configuration with change detection #1071

Merged
merged 47 commits into from
Jun 26, 2024

Conversation

attilakreiner
Copy link
Contributor

@attilakreiner attilakreiner commented May 31, 2024

Description

This change moves away from URL to Path abstraction and introduces the http file system, so the config and the resources can be watched over file and http protocol and opens up the potential for further extension.

Fixes #1061

@attilakreiner attilakreiner force-pushed the config-reload branch 2 times, most recently from 348e69f to e1d5642 Compare June 5, 2024 07:46
@attilakreiner attilakreiner changed the title WIP WIP Support remote zilla configuration with change detection Jun 5, 2024
@@ -144,7 +144,7 @@ public void attach(
Object2ObjectHashMap::new));

this.composite = namespaceGenerator.generate(binding, openapis, asyncapis, openapiSchemaIdsByApiId::get);
this.composite.readURL = binding.readURL;
this.composite.readLocation = binding.readLocation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to get rid of composite.readURL / composite.readLocation entirely?

@@ -27,7 +27,7 @@ public class BindingConfig
public transient long id;
public transient long entryId;
public transient ToLongFunction<String> resolveId;
public transient Function<String, String> readURL;
public transient Function<String, String> readLocation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to remove this completely?

import java.util.function.Function;

public class GuardConfig
{
public transient long id;
public transient Function<String, String> readURL;
public transient Function<Path, String> readPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed now?

public transient int id;
public transient Function<String, String> readURL;
public transient Function<String, String> readLocation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this one too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove it.

public ConfigWatcherTask(
FileSystem fileSystem,
Function<String, EngineConfig> configChangeListener,
Function<Path, String> readPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove readPath and use Files.readString(path) directly instead?

Comment on lines 59 to 67
String scheme = watchedPath.getFileSystem().provider().getScheme();
if ("file".equals(scheme))
{
registerFilePath();
}
else if ("http".equals(scheme))
{
watchKeys.add(watchedPath.register(watchService));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this more a question of optimization than specific schemes?

Copy link
Contributor Author

@attilakreiner attilakreiner Jun 18, 2024

Choose a reason for hiding this comment

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

There is different behaviour here.

In case of the file filesystem we want to watch files, but actually we can only watch directories. But our file can be a symlink or the directory can be a symlink or its parent etc... So Barnabas created a very sophisticated logic to cover all those cases, which means in a generic case we have to watch more than one object (directory) so we can make sure we are watching the changes of our file.

In case of http filesystem we just watch a URL and that's it. So it's a 1:1 mapping hence just one line of code here.

@@ -44,7 +44,7 @@ public JwtGuardHandler attach(
GuardConfig guard)
{
JwtOptionsConfig options = (JwtOptionsConfig) guard.options;
JwtGuardHandler handler = new JwtGuardHandler(options, context, supplyAuthorizedId, guard.readURL);
JwtGuardHandler handler = new JwtGuardHandler(options, context, supplyAuthorizedId, guard.readPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can JwtGuardHandler use EngineContext.readPath(...) instead of guard.readPath?

Comment on lines 48 to 60
WatchService watchService = null;
if (!"jar".equals(fileSystem.provider().getScheme())) // we can't watch in jar fs
{
try
{
watchService = fileSystem.newWatchService();
}
catch (Exception ex)
{
rethrowUnchecked(ex);
}
}
this.watchService = watchService;
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea of using a FileSystem abstraction is to move away from implementation specific knowledge of the filesystem as much as is practical in the engine.

The FileSystem.newWatchService() method is specified as an optional method, throwing UnsupportedOperationException on implementations not supporting it.

So we should be tolerant of that both now for jar scheme (used during tests) and later for other filesystems that also may not support newWatchService(), without needing to mention jar explicitly.

Suggested change
WatchService watchService = null;
if (!"jar".equals(fileSystem.provider().getScheme())) // we can't watch in jar fs
{
try
{
watchService = fileSystem.newWatchService();
}
catch (Exception ex)
{
rethrowUnchecked(ex);
}
}
this.watchService = watchService;
this.watchService = newWatchService(fileSystem);
        private static WatchService newWatchService(
            FileSystem fileSystem)
        {
            WatchService watcher = null;

            try
            {
                watcher = fileSystem.newWatchService();
            }
            catch (UnsupportedOperationException ex)
            {
                // no watcher
            }
            catch (Exception ex)
            {
                rethrowUnchecked(ex);
            }

            return watcher;
        }

Comment on lines 311 to 314
private String readLocation(
String location)
{
return readPath(resolvePath(location));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove readLocation everywhere, and use Files.readString(path) instead at the call sites.

We should no longer need BindingConfig.readLocation directly given that EngineWorker.resolvePath(location) is available to all handlers, so BindingConfig.readLocation can be removed as well.

Comment on lines 305 to 309
public Path resolvePath(
String location)
{
return configPath.resolveSibling(location);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This method implementation should move to EngineWorker instead of passing a lambda from Engine.

Note: EngineWorker already has EngineConfig and can call config.configPath() in EngineWorker constructor instead of passing as an explicit constructor parameter.

Comment on lines 194 to 196
this.configPath,
this::readPath,
this::readLocation);
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these need to be passed to EngineManager constructor.

Only configPath is needed and can be obtained from EngineConfiguration (already passed).

{
output = "";
result = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't want to default to empty string here.

After moving Files.readString(path) to each call site, handling an IOException will let the caller decide the best remedy instead of assuming empty string is always appropriate.

@@ -164,7 +148,7 @@ public final class Engine implements Collector, AutoCloseable
for (int workerIndex = 0; workerIndex < workerCount; workerIndex++)
{
EngineWorker worker =
new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, bindings, exporters,
new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, this::resolvePath, bindings, exporters,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, this::resolvePath, bindings, exporters,
new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, bindings, exporters,

Move resolvePath to EngineWorker implementation instead.

@@ -193,6 +177,7 @@ public final class Engine implements Collector, AutoCloseable
final Map<String, Guard> guardsByType = guards.stream()
.collect(Collectors.toMap(g -> g.name(), g -> g));

this.configPath = config.configPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like configPath is no longer needed as Engine field after resovlePath(...) method is moved to EngineWorker implementation directly.

Comment on lines 152 to 156
public Path configPath()
{
return ENGINE_CONFIG_PATH.get(this);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This configPath() method looks good.

Let's keep the ENGINE_CONFIG_URL constant in place for now, to support zilla.properties defining zilla.engine.config.url property, but remove all calls to EngineConfiguration.configURL() from the code, and replace with corresponding EngineConfiguration.configPath() method calls instead.

public transient int id;
public transient Function<String, String> readURL;
public transient Function<String, String> readLocation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove it.

Comment on lines 62 to 79
private List<String> resolveResources()
{
List<String> resources = new LinkedList<>();
for (CatalogConfig catalog : catalogs)
{
if (FILESYSTEM.equals(catalog.type))
{
resources.addAll(catalog.options.resources);
}
}
for (VaultConfig vault : vaults)
{
if (FILESYSTEM.equals(vault.type))
{
resources.addAll(vault.options.resources);
}
}
return resources;
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach assumes global knowledge of all possible implementations and assumes their behavior, such that a catalog or vault other than filesystem cannot contribute resources to be watched.

We need a more general strategy instead.

Please include all resources from any binding, guard, vault, catalog or telemetry exporter.

Note: instead of calling a non-static method from the constructor that relies on constructor field assignment, please make this method static and pass all required state as parameters, such as bindings, guards, vaults, catalogs, and telemetry.

Remove GuardedConfig.readPath
Rename ConfigAdapterContext.readLocation to ConfigAdapterContext.readResource
Deprecate ConfigAdapterContext for removal
Add EngineConfiguration.configURI() to resolve absolute file path
Remove EngineConfiguration.configURL() but use to default EngineConfiguration.configURI()
Simplify EngineManager
Gather resources on NamespaceConfig from member configs
Consolidate config and resource watcher as EngineConfigWatcher
Resolve watched paths based on path filesystem provider scheme
Configure HttpFileSystem poll internval duration via Map<String,?> env
Simplify ReconfigureHttpIT scripts and include /zilla.yaml in request path
HttpFileSystem per origin (root path) not per individual path
Track HttpPath change count vs read count to implement simple caching
@jfallows jfallows changed the title WIP Support remote zilla configuration with change detection Support remote zilla configuration with change detection Jun 26, 2024
@jfallows jfallows marked this pull request as ready for review June 26, 2024 04:41
jfallows
jfallows previously approved these changes Jun 26, 2024
@jfallows jfallows merged commit 230eec2 into aklivity:develop Jun 26, 2024
4 checks passed
@attilakreiner attilakreiner deleted the config-reload branch July 12, 2024 09:34
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.

Support remote zilla configuration with change detection
2 participants