Make the schema replication executor pool size configurable by ctc-louk · Pull Request #1487 · hazelcast/hazelcast-cpp-client

ctc-louk · 2026-09-02T15:57:06Z

Problem

ClientExecutionServiceImpl::start() created the schema replication thread pool with the no-argument constructor:

schema_replication_executor_.reset(new hazelcast::util::hz_thread_pool());

hz_thread_pool() delegates to boost::asio::thread_pool(), which sizes itself at hardware_concurrency() * 2 — 192 threads on a 96-core host. The pool's only consumer is ClientInvocation::replicate_schemas, which performs occasional compact schema replication, and a given schema is replicated at most once per client. The pool was therefore far larger than the workload warrants, sat idle in steady state, and there was no way to change it.

Change

Adds a client_properties entry:


Name	`hazelcast.client.schema.replication.pool.size`
Default	`3`
Resolution	`client_config::set_property()` > environment variable > default

start() resolves it exactly as it already resolves the internal executor pool size, clamping a non-positive value to the default.

hazelcast::client::client_config config;
config.set_property("hazelcast.client.schema.replication.pool.size", "2");

auto client = hazelcast::new_client(std::move(config)).get();

This follows every other pool knob in the client — IO_THREAD_COUNT, INTERNAL_EXECUTOR_POOL_SIZE, RESPONSE_THREAD_COUNT, EVENT_THREAD_COUNT — all of which are properties with small constant defaults. No public API is added, ClientExecutionServiceImpl's constructor is unchanged, and nothing is added to client_config, so there is no ABI impact.

The default of 3 replaces 2 * hardware_concurrency for every user, not only those who discover the knob. Tasks on this pool block — replicate_schema_in_cluster waits on invoke().get() and sleeps for the invocation retry pause on the retry path — so the property documents that a value of 1 makes concurrent writes of distinct unseen schemas serialize behind one another.

Testing

schema_replication_pool_size_test — offline; asserts the property resolves to the default when unset and that a configured value overrides it.
ThreadPoolTest.testSchemaReplicationPoolSizeIsHonoured — added to the existing ThreadPoolTest fixture, so it reuses that suite's member and adds no second HazelcastServer. Each parameter configures the pool to GetParam() and submits 2 * GetParam() jobs, asserting exactly GetParam() distinct thread ids. Every parameter differs from the default of 3, so each case fails if the property is ignored. The test uses a local hazelcast_client rather than the fixture's static raw pointer.

devOpsHazelcast · 2026-09-02T15:57:11Z

All committers have signed the CLA.

ihsandemir

Thanks for the PR. A few points, roughly in priority order.

Commit message: the commit d95f43e365 carries a Co-Authored-By: Claude ... trailer. Please amend and force-push before this is merged; the PR body itself is fine.

Mechanism: the Java client has no schema replication executor at all. ClientSchemaService.put replicates synchronously on the calling thread via invoke().joinInternal(), so there is nothing there to mirror. Every other pool knob in this client (IO_THREAD_COUNT, INTERNAL_EXECUTOR_POOL_SIZE, RESPONSE_THREAD_COUNT, EVENT_THREAD_COUNT) is a client_properties entry with a small constant default. A hazelcast.client.schema.replication.pool.size property with a default of 1-3 threads would need no constructor change, no client_config member, no ABI change, and would fix the 2 * hardware_concurrency default for every user instead of only those who discover the setter. A typed setter is public API we cannot remove during 5.x.

For what it is worth, I looked at whether the pool could be dropped entirely in favour of user_executor_ or internal_executor_. Neither is safe: complete_call_id_sequence() runs every invocation's completion continuation on user_executor_, and invocation retries are posted to internal_executor_ via ClientInvocation::execute(). replicate_schema_in_cluster blocks on invoke().get(), so running it on either pool can deadlock once the pool is full. The dedicated pool is justified; it just wants a small default.

Inline comments follow for the rest.

ihsandemir · 2026-09-03T07:49:03Z

+}
+
+void
+client_config::set_schema_replication_pool_size(


This stores any int32_t unvalidated. Only <= 0 is special-cased later in start(), so a value like 100000 reaches boost::asio::thread_pool(n) during client construction. If create_thread fails partway, ~thread_group joins the already-started workers blocked in run() with no stop(), so startup hangs or throws a raw std::system_error instead of exception::invalid_configuration.

set_executor_pool_size and friends already go through util::Preconditions::check_positive; one call here closes the gap.

Thanks for raising the issue of setting unrealistic large number of threads. The new approach follows existing pattern now.

ihsandemir · 2026-09-03T07:49:03Z

+    int32_t get_schema_replication_pool_size() const;
+
+    /**
+     * Sets the pool size for the schema replication executor. A value of 0 or


Worth stating the cost of a small value here. Every task on this pool blocks: replicate_schema_in_cluster calls invoke().get() and, in the retry branch, sleeps retry_pause_millis_ (1000 ms default) up to max_put_retry_count_ (100) times while holding the thread. The .then(boost::launch::sync) in ClientInvocation::invoke then runs invoke_on_selection() on that same thread. With size 1, two threads writing distinct unseen compact schemas serialize on each other, and in a split-brain case unrelated invocations can stall for up to ~100 s. Either document that or enforce a floor in the setter.

the new approach reverts the change on client_config.h

ihsandemir · 2026-09-03T07:49:03Z

+     *
+     * \param schema_replication_pool_size pool size
+     */
+    void set_schema_replication_pool_size(int32_t schema_replication_pool_size);


See the top-level comment: I would prefer a client_properties entry over a typed setter here. All sibling pool sizes are properties with small constant defaults, ClientExecutionServiceImpl already holds client_properties_, and this avoids a permanent public API and the constructor signature change. It also lets us pick a sane default (1-3) in the same PR rather than leaving 2 * hardware_concurrency in place.

the new approach reverts the change on client_config.h

ihsandemir · 2026-09-03T07:49:03Z

    ClientExecutionServiceImpl(const std::string& name,
                               const client_properties& properties,
                               int32_t user_pool_size,
+                               int32_t schema_replication_pool_size,


This is a HAZELCAST_API exported class and the new parameter is inserted mid-signature with no default, so the old 4-argument constructor disappears in a minor release. Two adjacent int32_t parameters also mean a transposed get_executor_pool_size() / get_schema_replication_pool_size() at the call site in client_impl.cpp compiles silently and mis-sizes both pools.

If the setter stays, appending int32_t schema_replication_pool_size = -1 as the last parameter keeps source compatibility for free. If this becomes a property, the constructor does not need to change at all.

the new approach reverts the change

ihsandemir · 2026-09-03T07:49:03Z

    }

-    schema_replication_executor_.reset(new hazelcast::util::hz_thread_pool());
+    if (schema_replication_pool_size_ <= 0) {


This is now the third copy of the if (size <= 0) reset(new hz_thread_pool()) else reset(new hz_thread_pool(size)) block in start(). The sentinel policy belongs in hz_thread_pool, which already owns the default/explicit constructor split. A single hz_thread_pool(int32_t n) that treats n <= 0 as default collapses each site to one line and removes the negative-to-size_t footgun for any caller that forgets the guard.

ihsandemir · 2026-09-03T07:49:03Z

+{
+    client_config config;
+
+    ASSERT_EQ(-1, config.get_schema_replication_pool_size());


This freezes the -1 sentinel into the public contract. We now have four descriptions of one state that already disagree: the field comment says -1 means unset, the setter doc says 0 or less, the consumer checks <= 0, and this test asserts exactly -1. The resolved default (2 * hardware_concurrency) is stated nowhere. Moving to a concrete default later becomes an observable API and test change.

Also, these plain TEST()s sit between the AWS tests. Every other client_config accessor is tested in the ClientConfigTest fixture in HazelcastTests2.cpp; they belong there.

the new approach reverts the change

ihsandemir · 2026-09-03T07:49:03Z

                         ThreadPoolTest,
                         ::testing::Values(5, 10, 2));

+class SchemaReplicationThreadPoolTest


This fixture is a verbatim copy of ThreadPoolTest directly above, differing only in the config setter and the executor getter, and it boots a second HazelcastServer. The two copies of ThreadState, SetUpTestCase/TearDownTestCase, the static server/client and the barrier/latch body will drift independently, and CI pays an extra member start plus three more client connects.

Parametrizing the existing fixture over {setter, executor getter} (or adding a second TEST_P to ThreadPoolTest) reuses the server and keeps one harness to maintain.

added a second TEST_P to ThreadPoolTest

ihsandemir · 2026-09-03T07:49:03Z

+    if (client != nullptr) {
+        client->shutdown().get();
+    }
+    client = new hazelcast_client{ new_client(std::move(config)).get() };


client is a static raw pointer. On the second and third parametrized runs the previous object is shut down and then the pointer is overwritten with a fresh new without a delete, so only the last of the three clients is freed in TearDownTestCase. shutdown() stops threads and connections but the hazelcast_client_instance_impl graph it owns stays allocated until the wrapper is destroyed, so two full impl graphs leak per suite run.

The pattern is inherited from ThreadPoolTest, so this PR doubles the count rather than introducing it. CI only misses it because scripts/test-unix.sh sets ASAN_OPTIONS=detect_leaks=0. A local auto client = new_client(std::move(config)).get(); in the test body removes the static, the teardown bookkeeping and the leak in one go.

ihsandemir · 2026-09-03T07:49:03Z

+    ASSERT_EQ(expected_thread_num, state->thread_ids.size());
+}
+
+INSTANTIATE_TEST_SUITE_P(SchemaReplicationThreadPoolTestSuite,


With num_of_thread hardcoded to 5, the jobs = 5 and jobs = 2 cases assert min(jobs, 5) distinct threads, which any pool of size >= jobs satisfies, including the unconfigured default of 2 * hardware_concurrency. If set_schema_replication_pool_size were silently ignored, the exact regression this test exists to catch, those two cases would still pass on any multi-core machine. Only jobs = 10 discriminates, and each vacuous case costs a full client connect. One case with jobs > threads asserting exactly 5 distinct thread ids covers the change.

ctc-louk · 2026-09-03T16:22:53Z

We (CTC) will need this feature sooner than the major version (6.x), ideally a minor version. I pivoted to the recommended approach of adding hazelcast.client.schema.replication.pool.size property

ihsandemir

Thanks. looks great. I will also send a documentation update PR after the PR is merged.

## Problem `actions/checkout` v6.1.0 (2026-07-20) refuses to check out fork pull request code from a `pull_request_target` workflow unless the step opts in: ``` Refusing to check out fork pull request code from a 'pull_request_target' workflow. ... To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step. ``` `build-pr.yml` uses the floating `@v6` tag, so since that release every external contributor PR fails in the checkout step of all build, coverage and formatting jobs once a member re-runs it. PRs from branches in this repository are unaffected, which is why it went unnoticed. Example: https://github.com/hazelcast/hazelcast-cpp-client/actions/runs/33777102057 (re-run of #1487). ## Change Add `allow-unsafe-pr-checkout: true` to the five `actions/checkout@v6` steps in `build-pr.yml`. All of these steps run downstream of the `ensure-membership` job, so untrusted fork code is only checked out when a Hazelcast organisation member triggered or re-ran the workflow, or on a manual dispatch with a verified commit. That is the gating the checkout documentation asks for before opting in. ## Testing YAML parses. The behaviour can only be verified by re-running an external PR's workflow after this lands on master, since `pull_request_target` reads the workflow from the base branch.

ihsandemir · 2026-09-04T11:27:18Z

Hello @ctc-louk can you rebase and push?

The schema replication thread pool was created with the no-argument hz_thread_pool() constructor, which delegates to boost::asio::thread_pool() and so sizes itself at hardware_concurrency() * 2 -- 192 threads on a 96-core host. Its only consumer is ClientInvocation::replicate_schemas, which performs occasional compact schema replication, so the pool sat far larger than the workload warrants with no way to change it. Add hazelcast.client.schema.replication.pool.size, following the existing pool knobs (IO_THREAD_COUNT, INTERNAL_EXECUTOR_POOL_SIZE, RESPONSE_THREAD_COUNT, EVENT_THREAD_COUNT), all of which are client_properties entries with small constant defaults. start() resolves it the same way it resolves the internal executor pool size, clamping a non-positive value to the default. The default of 3 replaces 2 * hardware_concurrency for every user rather than only those who opt in. Tasks on this pool block on invoke().get(), so the property is documented as needing more than one thread to keep concurrent writes of distinct schemas from serializing.

ctc-louk · 2026-09-04T13:44:13Z

Appreciate the timely review. I've rebased to the tip of master (b4dd58f).

ctc-louk requested a review from ihsandemir as a code owner September 2, 2026 15:57

ihsandemir assigned ctc-louk Sep 3, 2026

ihsandemir added Type: Enhancement Source: Community labels Sep 3, 2026

ihsandemir added this to the 6.0.0 milestone Sep 3, 2026

ihsandemir reviewed Sep 3, 2026

View reviewed changes

ctc-louk force-pushed the configurable-thread-pool branch 2 times, most recently from c31ef8b to d11dccf Compare September 3, 2026 15:43

ctc-louk marked this pull request as draft September 3, 2026 16:08

ctc-louk force-pushed the configurable-thread-pool branch from d11dccf to e0cad73 Compare September 3, 2026 16:11

ctc-louk marked this pull request as ready for review September 3, 2026 16:29

ihsandemir modified the milestones: 6.0.0, 5.7.1 Sep 4, 2026

ihsandemir approved these changes Sep 4, 2026

View reviewed changes

ihsandemir mentioned this pull request Sep 4, 2026

Allow fork PR checkout in the pull_request_target build workflow #1488

Merged

ihsandemir modified the milestones: 5.7.1, 6.0.0 Sep 4, 2026

ctc-louk force-pushed the configurable-thread-pool branch from e0cad73 to e9983b6 Compare September 4, 2026 13:32

ihsandemir approved these changes Sep 7, 2026

View reviewed changes

ihsandemir enabled auto-merge (squash) September 7, 2026 06:37

ihsandemir merged commit 96ec265 into hazelcast:master Sep 7, 2026
40 checks passed

Uh oh!

Conversation

ctc-louk commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Testing

Uh oh!

devOpsHazelcast commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ihsandemir left a comment

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!

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!

ctc-louk commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ihsandemir left a comment

Choose a reason for hiding this comment

Uh oh!

ihsandemir commented Sep 4, 2026

Uh oh!

ctc-louk commented Sep 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ctc-louk commented Sep 2, 2026 •

edited

Loading

devOpsHazelcast commented Sep 2, 2026 •

edited

Loading

ctc-louk commented Sep 3, 2026 •

edited

Loading