Support load balancer health check even at full capacity #1509

akrambek · 2025-06-23T04:58:09Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineBoss.java

jfallows · 2025-06-24T12:30:09Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineBoss.java

+        for (BindingConfig binding : namespace.bindings)
+        {
+            BindingController controller = controllersByType.get(binding.type);
+            if (controller != null)
+            {
+                controller.detach(binding);
+            }
+        }


I think we need to offer this as a NamespaceTask too, to make sure it executes on the boss thread, not the caller thread.

jfallows · 2025-06-24T12:30:44Z

...e/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineManager.java

    private void register(
        NamespaceConfig namespace)
    {
+        this.boss.attach(namespace).join();


Suggested change

this.boss.attach(namespace).join();

boss.attach(namespace).join();

jfallows · 2025-06-24T12:31:17Z

...e/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineManager.java

    {
        if (namespace != null)
        {
+            boss.detach(namespace);


Suggested change

boss.detach(namespace);

boss.detach(namespace).join();

jfallows · 2025-06-24T12:40:16Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineBoss.java

+    {
+        try
+        {
+            controllersByType.values().forEach(BindingController::detachAll);


This detachAll() for EngineBoss and BindingController seems inconsistent with the design of separation between EngineWorker and BindingHandler.

Instead, we can keep track of the NamespaceConfigs that we have attached to the EngineBoss and then iterate each NamespaceConfig to here in doClose and detach them all.

jfallows · 2025-06-24T12:49:31Z

...cp/src/main/java/io/aklivity/zilla/runtime/binding/tcp/internal/stream/TcpServerFactory.java

        if (route != null)
        {
-            final TcpServer server = new TcpServer(binding.id, route.id, network);
+            final TcpServer server = new TcpServer(bindingId, route.id, network);


Suggested change

final TcpServer server = new TcpServer(bindingId, route.id, network);

final TcpServer server = new TcpServer(binding.id, route.id, network);

We already used bindingId to resolve binding so at this point it is more correct to use binding.id.

jfallows · 2025-06-24T12:51:26Z

...cp/src/main/java/io/aklivity/zilla/runtime/binding/tcp/internal/stream/TcpClientFactory.java

        if (binding != null)
        {
-            route = router.resolve(binding, traceId, authorization, beginEx);
+            route = binding.resolve(binding, traceId, authorization, beginEx);


Suggested change

route = binding.resolve(binding, traceId, authorization, beginEx);

route = binding.resolve(traceId, authorization, beginEx);

jfallows · 2025-06-24T12:52:37Z

...cp/src/main/java/io/aklivity/zilla/runtime/binding/tcp/internal/config/TcpBindingConfig.java

+    public InetSocketAddress resolve(
+        TcpBindingConfig binding,
+        long traceId,
+        long authorization,
+        ProxyBeginExFW beginEx)
+    {
+        final TcpOptionsConfig options = binding.options;


Suggested change

public InetSocketAddress resolve(

TcpBindingConfig binding,

long traceId,

long authorization,

ProxyBeginExFW beginEx)

{

final TcpOptionsConfig options = binding.options;

public InetSocketAddress resolve(

long traceId,

long authorization,

ProxyBeginExFW beginEx)

{

jfallows · 2025-06-24T12:55:37Z

...inding-tcp/src/main/java/io/aklivity/zilla/runtime/binding/tcp/internal/TcpUsageTracker.java

    private final LongConsumer recordUsage;

-    private int usage;
+    private volatile int usage;


Did we change this to volatile to handle access from engine boss thread during tcp accept?

No sorry I was debugging and forgot to remove it

jfallows · 2025-06-24T12:56:05Z

...g-tcp/src/main/java/io/aklivity/zilla/runtime/binding/tcp/internal/TcpBindingController.java

+    @Override
+    public void detachAll()
+    {
+        serversById.values().forEach(CloseHelper::quietCloseAll);
+        serversById.clear();
+    }


Please remove this method.

jfallows · 2025-06-26T05:35:54Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineBoss.java

    }

-    public void detach(
+    private void detachNamespace(


Please move this private method down below attachNamespace.

jfallows · 2025-06-26T05:39:02Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineBoss.java

+    private void attachNamespace(
+        NamespaceConfig namespace)
+    {
+        namespaces.add(namespace);


This is good.

We also need symmetrical namespaces.remove(namespace); in detachNamespace method.

Note that namespaces can be added and removed while engine is still running, so detachNamespace is not only for doClose case, that's why we need to make sure it is kept accurate.

jfallows · 2025-06-26T05:42:39Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineBoss.java

+    public void attachNow(
+        NamespaceConfig namespace)
+    {
+        attach(namespace).join();
+    }
+


Do we still need attachNow and detachNow? If not, let's remove them.

jfallows · 2025-06-26T23:04:02Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineBoss.java

+
+public class EngineBoss implements EngineController, Agent
+{
+    private static final String AGENT_NAME = "EngineBoss";


Suggested change

private static final String AGENT_NAME = "EngineBoss";

private static final String AGENT_NAME = "engine/boss";

jfallows · 2025-06-26T23:05:28Z

...me/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineWorker.java

Change agentName to this.agentName = String.format("engine/worker#%d", index);

akrambek added 13 commits June 15, 2025 19:48

WIP

fe82f3d

WIP

aa3ef82

WIP

a73e31d

WIP

3c296de

WIP

73556d7

WIP

e8f39ac

WIP

21a7115

Fix typo

0bbbdac

Refactor tcp tracker

181f177

Fix remaining issues

1e33a48

Fix checkstyle

e04dcc0

Make tests consistent

66cf9bd

Fix the race

a60259d

akrambek marked this pull request as ready for review June 24, 2025 12:34

jfallows requested changes Jun 24, 2025

View reviewed changes

akrambek added 3 commits June 25, 2025 12:43

Apply feedback from PR

2c162ef

Fix checkstyle

78f1ac3

Fix composite

e597125

jfallows requested changes Jun 26, 2025

View reviewed changes

akrambek added 3 commits June 26, 2025 11:06

Apply feedback from PR

bab3ea4

Fix checkstyle

173a791

Revert back change and fix concurent modification exception

3f344fc

jfallows requested changes Jun 26, 2025

View reviewed changes

akrambek added 2 commits June 27, 2025 08:55

Apply feedback from PR

6746be8

Change the execution order

36d1db3

jfallows approved these changes Jun 28, 2025

View reviewed changes

jfallows changed the title ~~Load balancer support~~ Support load balancer health check even at full capacity Jun 28, 2025

jfallows merged commit cdee164 into aklivity:develop Jun 28, 2025
39 of 41 checks passed

	this.boss.attach(namespace).join();
	boss.attach(namespace).join();

	final TcpServer server = new TcpServer(bindingId, route.id, network);
	final TcpServer server = new TcpServer(binding.id, route.id, network);

	route = binding.resolve(binding, traceId, authorization, beginEx);
	route = binding.resolve(traceId, authorization, beginEx);

	private static final String AGENT_NAME = "EngineBoss";
	private static final String AGENT_NAME = "engine/boss";

Support load balancer health check even at full capacity #1509

Support load balancer health check even at full capacity #1509

Uh oh!

Conversation

akrambek commented Jun 23, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants