Make (some) Ice properties opt-in (C++ only) #2983

externl · 2024-10-28T14:56:24Z

This PR updates the generated PropertyNames to indicate if a property array belongs to a service. Properties which belong to a service can now no longer be set unless the corresponding Property object is constructed to enable the service's prefix.

I've also renamed UnknownPropertyException to PropertyException so we can use it for a few more things.

See #2855

…cope

externl · 2024-10-28T19:59:59Z

cpp/include/DataStorm/Node.h

-         * A node is the main DataStorm object. It is required to construct topics. The constructor initializes
-         * the Ice communicator using the given arguments. If the communicator creation fails, an Ice exception is
-         * raised.
+         * A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice


We needed to be able to intercept the construction of the Node's communicator to create a Property set with the "DataStorm" service prefix. So we can no longer just forward everything. @bernardnormier suggested we just offer a few constructors. This also means we can now always have the custom executor as a trailing parameter.

The alternative is to not consider DataStorm a service, which is not really one. It is a user library given that you have to programmatically construct the node.

In theory any servers communicator can be shared with the node and pass to its constructor.

I'd prefer making DataStorm an opt-in reserved prefix, as opposed to allowing it all the time like the plugins.

Isn't this the same as IceDiscovery and IceLocatorDiscovery which are marked with service-prefix=false.?

All the plugins (IceDiscovery etc.) are treated like Ice - always enabled.

externl · 2024-10-28T20:00:47Z

cpp/src/IceBox/ServiceManagerI.cpp

+            // Inherit all except IceBox. and Ice.Admin. properties
+            if (p.first.find("IceBox.") != 0 && p.first.find("Ice.Admin.") != 0)


We now also filter out IceBox. properties.

pepone · 2024-10-29T13:55:40Z

cpp/src/Ice/Properties.cpp

+        if (propertyArray->isServicePrefix &&
+            (_servicePrefix != propertyArray->name && propertyArray->name != string{"IceStorm"}))
+        {


Can't we have multiple servicePrefixes, instead of hardcoding IceStorm here?

Right now we only need one. And this hardcoding of IceStorm will go away one we remove it as an IceBox service.

Ins't it also required for IceStorm instance run by IceGrid?

IceGrid doesn't set any IceStorm properties AFAICT.

I'm going to update to a list.

I've updated to take list and removed the hard-coded "IceStorm".

pepone · 2024-10-29T13:57:45Z

cpp/include/DataStorm/Node.h

-         * A node is the main DataStorm object. It is required to construct topics. The constructor initializes
-         * the Ice communicator using the given arguments. If the communicator creation fails, an Ice exception is
-         * raised.
+         * A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice


The alternative is to not consider DataStorm a service, which is not really one. It is a user library given that you have to programmatically construct the node.

In theory any servers communicator can be shared with the node and pass to its constructor.

bernardnormier · 2024-10-29T14:05:36Z

cpp/include/DataStorm/Node.h

+        /**
+         * Construct a DataStorm node.
+         *
+         * A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice


This constructor (like most Node constructors) does not accept a communicator. So:

The node uses the given Ice communicator.

is not correct.

bernardnormier · 2024-10-29T14:06:43Z

cpp/include/DataStorm/Node.h

@@ -133,7 +201,10 @@ namespace DataStorm
        Ice::ConnectionPtr getSessionConnection(const std::string& ident) const noexcept;

    private:
-        void init(const Ice::CommunicatorPtr&, std::function<void(std::function<void()> call)> customExecutor);
+        Node(
+            const Ice::CommunicatorPtr&,


Should be an Ice::CommunicatorPtr.

bernardnormier · 2024-10-29T14:29:31Z

cpp/include/DataStorm/Node.h

-         * A node is the main DataStorm object. It is required to construct topics. The constructor initializes
-         * the Ice communicator using the given arguments. If the communicator creation fails, an Ice exception is
-         * raised.
+         * A node is the main DataStorm object. It is required to construct topics. The node uses the given Ice


I'd prefer making DataStorm an opt-in reserved prefix, as opposed to allowing it all the time like the plugins.

pepone · 2024-10-29T17:48:28Z

cpp/src/DataStorm/Node.cpp

+        Ice::InitializationData initData;
+        initData.properties = properties;
+
+        return Ice::initialize(argc, argv, initData);


Can't we instead add an extra opt-in prefixes to Ice::initialize, and use that?

If I understand correctly, you're thinking an extra overload of args, argv, optInPrefixes?

We already have too many initialize.

The opt-in prefixes are primarily for services that we write. It will be extremely rare for a user to opt-in "IceStorm" or "DataStorm". So I'd rather not provide a more overloads to make this more convenient.

cpp/include/Ice/Properties.h

…+ only) (zeroc-ice/ice#2983) Updates the generated PropertyNames to indicate if a property array is opt-in. Properties whose prefix is opt-in can now no longer be set unless the corresponding Property object is constructed with an opt-in list containing its prefix. I've also renamed UnknownPropertyException to PropertyException so we can use it for a few more things. Part of zeroc-ice/ice#2855

externl added 10 commits October 22, 2024 14:28

wip

feaa7da

Merge remote-tracking branch 'origin/main' into 2855-limit-property-s…

884292e

…cope

Fixes

5544078

Merge remote-tracking branch 'origin/main' into 2855-limit-property-s…

ca57c54

…cope

Build fixes

f0ba1d2

Rename UnknownPropertyException

79ce92c

Merge branch 'main' into 2855-limit-property-scope

5065e0b

Windows fix

22b772c

Fix test

8a575a3

Properties clone fix

a50f850

externl changed the title ~~2855 limit property scope~~ Add service properties in C++ Oct 28, 2024

externl commented Oct 28, 2024

View reviewed changes

Merge branch 'main' into 2855-limit-property-scope

cf53d1c

externl requested review from pepone and bernardnormier and removed request for pepone October 29, 2024 12:41

externl marked this pull request as ready for review October 29, 2024 13:30

pepone reviewed Oct 29, 2024

View reviewed changes

externl mentioned this pull request Oct 29, 2024

Make IceStorm a regular binary service #3000

Open

bernardnormier reviewed Oct 29, 2024

View reviewed changes

Fixes

5a33ad2

externl changed the title ~~Add service properties in C++~~ Make (some) Ice properties opt-in (C++ only) Oct 29, 2024

externl requested review from pepone and bernardnormier October 29, 2024 16:12

Review fix

eac6ffb

pepone reviewed Oct 29, 2024

View reviewed changes

bernardnormier approved these changes Oct 29, 2024

View reviewed changes

cpp/include/Ice/Properties.h Outdated Show resolved Hide resolved

review fix

bbe4193

Merge branch 'main' into 2855-limit-property-scope

7b669ca

pepone approved these changes Oct 29, 2024

View reviewed changes

externl merged commit 6499167 into zeroc-ice:main Oct 30, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make (some) Ice properties opt-in (C++ only) #2983

Make (some) Ice properties opt-in (C++ only) #2983

externl commented Oct 28, 2024 •

edited

Loading

externl Oct 28, 2024 •

edited

Loading

pepone Oct 29, 2024

bernardnormier Oct 29, 2024

pepone Oct 29, 2024

bernardnormier Oct 29, 2024

externl Oct 28, 2024

pepone Oct 29, 2024

externl Oct 29, 2024 •

edited

Loading

pepone Oct 29, 2024

externl Oct 29, 2024 •

edited

Loading

externl Oct 29, 2024

externl Oct 29, 2024

pepone Oct 29, 2024

bernardnormier Oct 29, 2024

externl Oct 29, 2024

bernardnormier Oct 29, 2024

bernardnormier Oct 29, 2024

pepone Oct 29, 2024 •

edited

Loading

externl Oct 29, 2024 •

edited

Loading

bernardnormier Oct 29, 2024

		// Inherit all except IceBox. and Ice.Admin. properties
		if (p.first.find("IceBox.") != 0 && p.first.find("Ice.Admin.") != 0)

Make (some) Ice properties opt-in (C++ only) #2983

Make (some) Ice properties opt-in (C++ only) #2983

Conversation

externl commented Oct 28, 2024 • edited Loading

externl Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

externl Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

externl Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

externl Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

externl commented Oct 28, 2024 •

edited

Loading

externl Oct 28, 2024 •

edited

Loading

externl Oct 29, 2024 •

edited

Loading

externl Oct 29, 2024 •

edited

Loading

pepone Oct 29, 2024 •

edited

Loading

externl Oct 29, 2024 •

edited

Loading