Feat[MQB]: Support listening on multiple ports #204

hallfox · 2024-02-16T23:55:00Z

This extends the broker config to support listening to connections on multiple ports. This lays the groundwork for future extensions that will be added to TcpInterfaceListener. The broker will behave the same without the listener config, but with the listeners added the broker will open new listen operations for each one, ignoring the port field in the networkInterfaces. We also added an extra component for validating these new configs.

src/groups/mqb/mqbnet/mqbnet_transportmanager.cpp

dorjesinpo

Looks good and straightforward
Few comments, and probably will need to take another look after tests are fixed

dorjesinpo · 2024-02-20T17:10:30Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

@@ -661,7 +751,7 @@ void TCPSessionFactory::channelStateCallback(
    // is fine to do this here (since we have no other ways to
    // proactively-execute code in the IO threads created by the channelPool).
    if (mwcsys::ThreadUtil::k_SUPPORT_THREAD_NAME) {
-        mwcsys::ThreadUtil::setCurrentThreadNameOnce(d_threadName);
+        mwcsys::ThreadUtil::setCurrentThreadNameOnce(d_config.name());


d_threadName = "bmqIO_" + d_config.name().substr(0, 15 - 6);
What happens with the prefix? And with the d_threadName?

This was a mistake, reverting this.

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

dorjesinpo · 2024-02-20T17:27:33Z

src/groups/mqb/mqbcfg/mqbcfg.xsd

+    </annotation>
+    <sequence>
+      <element name='name'                type='string'/>
+      <element name='port'                type='int'/>


We may need to retire the above port when all broker versions read new config.
We need to mark it as deprecated maybe?

Yep, I can mark it deprecated in the schema documentation. The behavior of what the broker should do is probably better documented in other documentation sources too.

dorjesinpo · 2024-02-20T18:58:13Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

-    OperationContext* context = new (*d_allocator_p) OperationContext();
-    d_listenContext_mp.load(context, d_allocator_p);
-    bsl::shared_ptr<OperationContext> contextSp;
-    contextSp.reset(context, bslstl::SharedPtrNilDeleter());


Yeah, this insisting on the ManagedPtr looks strange

Yeah I'm not in love with this pattern of trading around ManagedPtrs as references wrapped in a shared_ptr, either way now there's a reason to make it shared so this doesn't even work anymore 😄

pniedzielski · 2024-02-22T17:14:25Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.h

@@ -528,7 +532,8 @@ class TCPSessionFactory {
    /// Listen to the specified `port` for incoming connection and invoke


The documentation comment here is out-of-date now: port argument replaced with listener; sentence should be changed.

pniedzielski · 2024-02-22T17:20:35Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.h

@@ -468,6 +465,13 @@ class TCPSessionFactory {
    /// event scheduler processes it.
    void disableHeartbeat(const bsl::shared_ptr<ChannelInfo>& channelInfo);

+    /// \brief Check that the TCP interfaces are valid.


This is our first Doxygen \brief. Two things:

CONTRIBUTING.md says we should use Javadoc style @brief.

We currently have Doxygen AUTOBRIEF turned off, but it seems likely we'll need to turn it on when we start using straight Doxygen rather than BDE-flavor docs, or we'll need to go through every docstring and add @brief (yikes!). For the moment, it seems better to match the style of the rest of the docs here.

pniedzielski · 2024-02-22T17:21:52Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.h

-    // Handle which can be used to
-    // stop listening.  Empty unless
-    // listening.
+    // Handles which can be used to stop listening.  Empty unless listening.


/// ? Change above uses Doxygen style. Seems like it's best to use Doxygen style throughout, and let Doxygen itself choose whether to output docs on private members.

pniedzielski · 2024-02-22T17:23:24Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.h

-    // Handle which can be used to
-    // stop listening.  Empty unless
-    // listening.
+    // Handles which can be used to stop listening.  Empty unless listening.


Handles which -> Handles that---had to look at the type to understand that handles isn't a verb here.

pniedzielski · 2024-02-22T17:28:30Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

+                errorDescription << "Failed listening to port '" << it->port()
+                                 << "' for TCPSessionFactory '"
+                                 << d_config.name() << "' [rc: " << rc << "]";
+                return rc;  // RETURN


Do we need cleanup for ports in earlier iterations of the loop that we have successfully listened on? Prior to this patch this wasn't a concern.

pniedzielski · 2024-02-22T17:35:15Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

+
+        allocator_type get_allocator() const { return d_allocator; }
+
+        /// viewFn :: NetworkInterface -> T


Haskell type notation 😂 But I'm not sure this is a useful docstring.

But had to look at the code to understand that T is an arbitrary Regular type to be used as a comparison key. More docs around the use and purpose of this private class, as well this function particularly, would be helpful.

pniedzielski · 2024-02-22T17:35:58Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

+    UniqueProperty d_unique;
+
+    // PRIVATE STATIC FUNCTIONS
+    static int portView(const mqbcfg::TcpInterfaceListener& listener)


Is this a view?

It could create a view, so technically no. The codegen'd API doesn't give me const ref access to this field, so it's a little unfortunate, but It's sufficient enough for our purposes.

pniedzielski · 2024-02-22T18:59:26Z

src/groups/mqb/mqbcfg/mqbcfg_messages.h


+class TcpInterfaceListener {


Documentation for publicly exposed class?

chrisbeard · 2024-07-30T17:42:49Z

src/groups/mqb/mqbcfg/doc/mqbcfg.txt

@@ -15,6 +15,7 @@ broker, as well as the generated corresponding messages component.
 ..
  1. mqbcfg_messages
  2. mqbcfg_brokerconfig
+  3. mqbcfg_tcpinterfaceconfigvalidator


nit: This adds another component/level, the synopsis can be updated

pniedzielski

Merge conflicts, can review again when you fix them.

- Add a new field for the broker config to list TCP interfaces with a port and name - Change how the port configuration is read to ignore the port field on the TCP interface when the new listeners are specified - Add a test fixture to support configuring clusters with multiple TCP interfaces open - Add an integration test to verify end-to-end connection on a cluster with multiple ports Signed-off-by: Taylor Foxhall <[email protected]>

bmq-oss-ci

Build 342 of commit de2f583 has completed with FAILURE

pniedzielski

let's ship it

hallfox requested a review from a team as a code owner February 16, 2024 23:55

hallfox force-pushed the multi-session-listener branch 3 times, most recently from 1046055 to 679a99f Compare February 20, 2024 17:56

678098 reviewed Feb 20, 2024

View reviewed changes

src/groups/mqb/mqbnet/mqbnet_transportmanager.cpp Show resolved Hide resolved

hallfox force-pushed the multi-session-listener branch from 679a99f to 3cb6921 Compare February 20, 2024 18:06

dorjesinpo reviewed Feb 20, 2024

View reviewed changes

hallfox force-pushed the multi-session-listener branch from 3cb6921 to 394b4d9 Compare February 20, 2024 19:30

pniedzielski reviewed Feb 22, 2024

View reviewed changes

hallfox force-pushed the multi-session-listener branch from 394b4d9 to db65221 Compare February 22, 2024 22:23

hallfox force-pushed the multi-session-listener branch from db65221 to 8b8f83a Compare May 20, 2024 20:00

hallfox changed the title ~~Support listening on open ports~~ Support listening on multiple ports Jul 25, 2024

hallfox force-pushed the multi-session-listener branch 5 times, most recently from 38396bf to 4bb16fd Compare July 30, 2024 15:15

chrisbeard reviewed Jul 30, 2024

View reviewed changes

hallfox force-pushed the multi-session-listener branch 9 times, most recently from ad17ee8 to 67b14e6 Compare August 8, 2024 18:35

hallfox force-pushed the multi-session-listener branch from 67b14e6 to 144b59e Compare August 21, 2024 21:58

pniedzielski self-requested a review August 26, 2024 13:39

pniedzielski previously approved these changes Aug 26, 2024

View reviewed changes

hallfox dismissed pniedzielski’s stale review via 43ae680 October 29, 2024 18:41

hallfox force-pushed the multi-session-listener branch from 144b59e to 43ae680 Compare October 29, 2024 18:41

hallfox changed the title ~~Support listening on multiple ports~~ Feat: Support listening on multiple ports Oct 29, 2024

hallfox changed the title ~~Feat: Support listening on multiple ports~~ Feat[MQB]: Support listening on multiple ports Oct 29, 2024

pniedzielski force-pushed the multi-session-listener branch from 43ae680 to de2f583 Compare October 29, 2024 19:23

bmq-oss-ci bot reviewed Oct 29, 2024

View reviewed changes

pniedzielski approved these changes Oct 29, 2024

View reviewed changes

hallfox merged commit e4bf5f6 into bloomberg:main Oct 30, 2024
35 checks passed

hallfox deleted the multi-session-listener branch October 30, 2024 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat[MQB]: Support listening on multiple ports #204

Feat[MQB]: Support listening on multiple ports #204

hallfox commented Feb 16, 2024 •

edited

Loading

dorjesinpo left a comment

dorjesinpo Feb 20, 2024

hallfox Feb 20, 2024

dorjesinpo Feb 20, 2024

hallfox Feb 20, 2024

dorjesinpo Feb 20, 2024

hallfox Feb 20, 2024

pniedzielski Feb 22, 2024

pniedzielski Feb 22, 2024

pniedzielski Feb 22, 2024

pniedzielski Feb 22, 2024

pniedzielski Feb 22, 2024

pniedzielski Feb 22, 2024

pniedzielski Feb 22, 2024

hallfox Feb 22, 2024

pniedzielski Feb 22, 2024

chrisbeard Jul 30, 2024

pniedzielski left a comment

bmq-oss-ci bot left a comment

pniedzielski left a comment

		@@ -528,7 +532,8 @@ class TCPSessionFactory {
		/// Listen to the specified `port` for incoming connection and invoke


		allocator_type get_allocator() const { return d_allocator; }

		/// viewFn :: NetworkInterface -> T

Feat[MQB]: Support listening on multiple ports #204

Feat[MQB]: Support listening on multiple ports #204

Conversation

hallfox commented Feb 16, 2024 • edited Loading

dorjesinpo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pniedzielski left a comment

Choose a reason for hiding this comment

bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

pniedzielski left a comment

Choose a reason for hiding this comment

hallfox commented Feb 16, 2024 •

edited

Loading