Skip to content

Conversation

zpqrtbnk
Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 18, 2023

This PR implements the core dynamic configuration support with all codecs etc + the map and ringbuffer configuration user code. Anything else is not part of this PR as it will quite probably be exposed via a REST API and not via dynamic configuration fluent API. Users can do, for instance:

await client.DynamicOptions.ConfigureMapAsync("map-name", mapOptions => 
{
    map.EventJournal.Enabled = true;
});

Note

  • in order to be "C# idiomatic" and to align with .NET naming everything "config" is "options" in .NET code
  • the codecs have been rebuilt with the latest protocol changes which means that a few things had to be adusted here and there in unrelated code as well.

We have tests to ensure that (1) the server would accept a complete configuration and (2) Java and .NET would produce the very same client message for the default dynamic configuration.

There are also a number of changes to files due to our public API surface going a bit out-of-control in the past PRs. In theory we have files that define our public API and detect any unwanted changes, but this detection seems to have been ignored in past PRs so it's all out of sync. Bringing it back in sync here.

NOTE: before merging this PR, ensure that hazelcast/hazelcast-client-protocol#496 is merged and update the state of the protocol sub-repository accordingly.

NOTE: as part of the merge, ensure that all v5.3.1 tag and 5.3.z branch have been properly merged into master.

Copy link

netlify bot commented Dec 18, 2023

Deploy Preview for silly-valkyrie-e996d9 failed.

Name Link
🔨 Latest commit 7971683
🔍 Latest deploy log https://app.netlify.com/sites/silly-valkyrie-e996d9/deploys/65b24170bfdca8000899f6b7

@zpqrtbnk zpqrtbnk requested a review from emreyigit December 18, 2023 15:48
@zpqrtbnk zpqrtbnk added this to the 5.4.0 milestone Dec 18, 2023
@zpqrtbnk zpqrtbnk added the State: Review Please review this PR! label Dec 18, 2023
@zpqrtbnk zpqrtbnk changed the title Implement dynamic configuration / map Implement dynamic configuration / map [API-2175] Dec 18, 2023
@zpqrtbnk zpqrtbnk self-assigned this Dec 18, 2023
@zpqrtbnk zpqrtbnk changed the title Implement dynamic configuration / map [API-2175] Implement dynamic configuration / map+ringbuffer [API-2175] Dec 20, 2023
@codecov-commenter
Copy link

Codecov Report

Attention: 141 lines in your changes are missing coverage. Please review.

Comparison is base (5655fe9) 84.99% compared to head (edc4ed9) 84.35%.

Files Patch % Lines
src/Hazelcast.Net/Models/Capacity.cs 10.00% 27 Missing ⚠️
src/Hazelcast.Net/NearCaching/NearCacheOptions.cs 67.60% 23 Missing ⚠️
src/Hazelcast.Net/Models/IndexOptions.cs 38.46% 16 Missing ⚠️
...st.Net/Protocol/BuiltInCodecs/CustomTypeFactory.cs 0.00% 15 Missing ⚠️
...t.Net/Protocol/Codecs/ClientAuthenticationCodec.cs 0.00% 12 Missing ⚠️
...Protocol/Codecs/ClientAuthenticationCustomCodec.cs 0.00% 12 Missing ⚠️
src/Hazelcast.Net/Models/BitmapIndexOptions.cs 52.38% 10 Missing ⚠️
src/Hazelcast.Net/Models/BTreeIndexOptions.cs 47.05% 9 Missing ⚠️
src/Hazelcast.Net/Models/MemoryTierOptions.cs 46.66% 8 Missing ⚠️
...rc/Hazelcast.Net/Protocol/Codecs/MapDeleteCodec.cs 0.00% 7 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #865      +/-   ##
==========================================
- Coverage   84.99%   84.35%   -0.65%     
==========================================
  Files         914      913       -1     
  Lines       21618    21815     +197     
==========================================
+ Hits        18375    18402      +27     
- Misses       3243     3413     +170     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@emreyigit emreyigit left a comment

Choose a reason for hiding this comment

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

I'm ok with the PR generally. I've left a few comments. Also, as I can see there are options but cannot be set or used yet. We should decrease their access modifiers since they are not avaiable tough declared as public. Also, excluding them from tests could be good idea until related implementation completed. They decrease the overall coverage.

emreyigit
emreyigit previously approved these changes Jan 16, 2024
@zpqrtbnk zpqrtbnk merged commit 319c888 into hazelcast:master Jan 25, 2024
@zpqrtbnk zpqrtbnk deleted the dynamic-configuration branch January 25, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Review Please review this PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants