Enhance bmqtool to handle poison pill message dumps #288

alexander-e1off · 2024-05-14T10:15:00Z

Enhance bmqtool to add ability to read and replay the poison pill message dumped in the file by the broker, so that consumers can debug their applications.

Proposed changes
Format of the dump file is defined in QueueEngineUtil::dumpMessageInTempfile() method. Since message properties are dumped as a string, it is impossible to parse them properly. Thus, it is proposed to enhance dump, and add properties hexdump (similar way as payload).

Add new command to bmqtool:
load-post uri=<uri> file=<path-to-dump-file>
Message content is loaded from dump file and posted to the specified uri.

Testing performed
Unit tests for parsing of dump file are added.
Manually with bmqtool simulated poison pill conditions and created dumps with and w/o message properties. Then tested these dumps with load-post commands.

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

…nto poisonpill

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

alexander-e1off · 2024-05-15T13:17:46Z

src/applications/bmqtool/m_bmqtool_interactive.cpp

@@ -474,11 +578,8 @@ void Interactive::processCommand(const PostCommand& command, bool hasMPs)

        if (hasMPs) {
            d_session_p->loadMessageProperties(&properties);
-
-            for (size_t j = 0; j < command.messageProperties().size(); ++j) {


Not sure I understand what is the purpose of this for cycle. Inside InputUtil::populateProperties() there is also for cycle which is used to process properties. Removed it.

678098

A few comments

src/applications/bmqtool/bmqtoolcmd.xsd

678098 · 2024-05-15T14:53:04Z

src/applications/bmqtool/m_bmqtool_inpututil.cpp

@@ -149,11 +150,55 @@ void InputUtil::populateProperties(
            BSLS_ASSERT_SAFE(0 == result);
        } break;  // BREAK

-        case MessagePropertyType::E_INT: {
+        case MessagePropertyType::E_INT32: {
            BSLA_MAYBE_UNUSED int result =


Here, and in other places:

Suggested change

BSLA_MAYBE_UNUSED int result =

BSLA_MAYBE_UNUSED const int result =

678098 · 2024-05-15T14:56:44Z

src/applications/bmqtool/m_bmqtool_inpututil.cpp

@@ -149,11 +150,55 @@ void InputUtil::populateProperties(
            BSLS_ASSERT_SAFE(0 == result);
        } break;  // BREAK

-        case MessagePropertyType::E_INT: {
+        case MessagePropertyType::E_INT32: {
            BSLA_MAYBE_UNUSED int result =


Might be also good to replace result -> rc to make it shorter and easier everywhere

678098 · 2024-05-15T15:04:56Z

src/applications/bmqtool/m_bmqtool_inpututil.cpp

+
+        case MessagePropertyType::E_BOOL: {
+            bool boolValue;
+            bsl::istringstream(value) >> bsl::boolalpha >> boolValue;


Line 166: uninitialized bool
Line 167: try to read bool value without check that the read reads something
Line 168: setting possibly random value of this bool as property

Maybe something like this will work:

bsl::stringstream iss(value); BSLA_MAYBE_UNUSED const bool readSuccess = (iss >> bsl::boolalpha >> boolValue); BSLS_ASSERT(readSuccess); ...

678098 · 2024-05-15T15:06:23Z

src/applications/bmqtool/m_bmqtool_inpututil.cpp

@@ -149,11 +150,55 @@ void InputUtil::populateProperties(
            BSLS_ASSERT_SAFE(0 == result);


I think BSLS_ASSERT_SAFE is not enough here, since we deal with user provided data here. When compiled as Release, we still want to have these values checks.

Consider using BSLS_ASSERT instead

678098 · 2024-05-15T19:13:24Z

src/applications/bmqtool/m_bmqtool_inpututil.h

@@ -80,6 +88,13 @@ struct InputUtil {
                             bsl::string*       error,
                             const bsl::string& jsonInput);

+    /// Decode hexdump produced by bdlb::Print::hexDump() into binary format.
+    /// Read hexdump from the specified `in` and write binary into th specified


Suggested change

/// Read hexdump from the specified `in` and write binary into th specified

/// Read hexdump from the specified `in` and write binary into the specified

678098 · 2024-05-15T19:15:57Z

src/applications/bmqtool/m_bmqtool_interactive.cpp

+bool loadMessageContentFromFile(
+    bsl::vector<bsl::string>*     payload,
+    bsl::vector<MessageProperty>* messageProperties,
+    bsl::ostream&                 errorDescription,


In some places, bsl::string * is passed to return error messages, here bsl::ostream.
Might be good to unify it. I believe passing bsl::ostream might be better, since it will be easier to output variable parameters like tokens, strings etc without intermediate mwcu::MemOutStream. What do you think?

678098 · 2024-05-15T19:18:51Z

src/applications/bmqtool/m_bmqtool_interactive.cpp

+                                        &parseError)) {
+            errorDescription << "Message properties parse error: "
+                             << parseError;
+            return false;


Here, and in other places below

Suggested change

return false;

return false; // RETURN

678098 · 2024-05-15T19:21:12Z

src/applications/bmqtool/m_bmqtool_interactive.cpp

@@ -900,7 +1027,15 @@ int Interactive::mainLoop()
                    if (mps) {
                        hasMPs = keys.find(mps->name()) != keys.cend();
                    }
-                    processCommand(command, hasMPs);
+
+                    bool commandValid = true;


Suggested change

bool commandValid = true;

bool commandIsValid = true;

678098 · 2024-05-15T19:22:00Z

src/applications/bmqtool/m_bmqtool_interactive.cpp

+                    }
+                    if (commandValid) {
+                        processCommand(command, hasMPs);
+                    }


Should we log something if the command is not valid?

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

alexander-e1off · 2024-05-21T14:25:23Z

@678098 What do you think about the following idea. To avoid reinventing the wheel during dump/parse message properties and keep existing functionality (human readable text) we can add properties hexdump using existing functionality:

During the message dump to the file, in addition to properties print, serialize properties into blob and hexdump them to the file the same way as payload is dumped;
When reading dump from the file, decode hexdump to binary and deserialize message properties;

In this case we keep dump file human readable (just one more hexdump will be added) and no need to create new format of properties dump. Below is simplified prototype

            // During message dump add properties hexdump
            bdlbb::PooledBlobBufferFactory bufferFactory(1024);
            const bdlbb::Blob& blob = properties.streamOut(&bufferFactory);
            dumpFileStream << bdlbb::BlobUtilHexDumper(&blob); 
        

            // When reading dump from the file
            mwcu::MemOutStream  binaryStream;
            bsl::istringstream inpStream(hexdumpStream.str());
            InputUtil::decodeHexDump(&binaryStream, inpStream)

            bdlbb::Blob resultBlob(&bufferFactory);
            bdlbb::BlobUtil::append(&resultBlob, binaryStream.str().data(), binaryStream.str().length());

            bmqa::MessageProperties properties;
            properties.streamIn(resultBlob);

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

alexander-e1off · 2024-05-24T16:16:12Z

@678098 Would you please look again, I've rolled back previous solution and implemented new one, also added unit tests. Thank you.

678098 · 2024-05-24T16:22:19Z

@678098 Would you please look again, I've rolled back previous solution and implemented new one, also added unit tests. Thanks you.

Hi @alexander-e1off, thanks for the changes! Will look soon

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

678098

Looking good, a few smaller suggestions

678098 · 2024-05-28T13:39:22Z

src/applications/bmqtool/bmqtoolcmd.xsd

+      <element name='uri'                      type='string'/>
+      <element name='file'                     type='string'/>


Suggested change

<element name='uri' type='string'/>

<element name='file' type='string'/>

<element name='uri' type='string'/>

<element name='file' type='string'/>

678098 · 2024-05-28T13:40:30Z

src/applications/bmqtool/m_bmqtool_inpututil.cpp

+
+    while (bsl::getline(in, line)) {
+        if (line.empty()) {
+            break;  // stop at the end of hexdump (empty line)


Suggested change

break; // stop at the end of hexdump (empty line)

// stop at the end of hexdump (empty line)

break; // BREAK

678098 · 2024-05-28T13:46:36Z

src/applications/bmqtool/m_bmqtool_inpututil.h

+                              bsl::istream&     in,
+                              bslma::Allocator* allocator);
+
+    /// Load message content from the specified `file` (created by


Suggested change

/// Load message content from the specified `file` (created by

/// Load message content from the specified `filePath` (created by

678098 · 2024-05-28T13:48:11Z

src/applications/bmqtool/m_bmqtool_inpututil.t.cpp

+// BMQ
+#include <bdlbb_pooledblobbufferfactory.h>
+#include <bmqa_messageproperties.h>
+#include <mwcu_memoutstream.h>
+#include <mwcu_tempfile.h>
+
+// BDE
+#include <bdlbb_blob.h>
+#include <bdlbb_blobutil.h>
+
+// TEST DRIVER
+#include <mwctst_testhelper.h>


Suggested change

// BMQ

#include <bdlbb_pooledblobbufferfactory.h>

#include <bmqa_messageproperties.h>

#include <mwcu_memoutstream.h>

#include <mwcu_tempfile.h>

// BDE

#include <bdlbb_blob.h>

#include <bdlbb_blobutil.h>

// TEST DRIVER

#include <mwctst_testhelper.h>

// BMQ

#include <bmqa_messageproperties.h>

// MWC

#include <mwcu_memoutstream.h>

#include <mwcu_tempfile.h>

// BDE

#include <bdlbb_blob.h>

#include <bdlbb_blobutil.h>

#include <bdlbb_pooledblobbufferfactory.h>

// TEST DRIVER

#include <mwctst_testhelper.h>

678098 · 2024-05-28T14:00:07Z

src/applications/bmqtool/m_bmqtool_interactive.cpp

@@ -159,6 +159,12 @@ void Interactive::printHelp()
        << "    - 'batch-post' command requires 'uri' argument, "
           "all the rest are optional"
        << bsl::endl
+        << bsl::endl
+        << "  load-post uri=\"bmq://bmq.test.persistent.priority/qqq\" "
+           "file=[\"message.dump\"]"


Do we need to specify file path in [] brackets? It is the same string type as uri, right?

678098 · 2024-05-28T14:04:04Z

src/applications/bmqtool/m_bmqtool_interactive.cpp

+
+    BALL_LOG_INFO << "--> Loading and posting message: " << command;
+
+    int rc;


Might be moved and const initialized in place below

Suggested change

int rc;

678098 · 2024-05-28T14:04:17Z

src/applications/bmqtool/m_bmqtool_interactive.cpp

+    }
+
+    // Post
+    rc = d_session_p->post(eventBuilder.messageEvent());


Suggested change

rc = d_session_p->post(eventBuilder.messageEvent());

const int rc = d_session_p->post(eventBuilder.messageEvent());

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

678098

LGTM!

alexander-e1off and others added 4 commits May 10, 2024 18:26

Add loading of message from dump file

d710c48

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Enhance verifyProperties()

42ae49c

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Remove file arg from BatchPostCommand

722f913

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Merge branch 'bloomberg:main' into poisonpill

1d604c7

alexander-e1off requested a review from a team as a code owner May 14, 2024 10:15

alexander-e1off changed the title ~~Enhance bmqtool to read poison pill message dumps~~ WIP: Enhance bmqtool to read poison pill message dumps May 14, 2024

alexander-e1off added 8 commits May 14, 2024 15:58

Cleanup

763683c

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Merge branch 'poisonpill' of https://github.com/waldgange/blazingmq i…

983dda7

…nto poisonpill

Fix style format

3f28dca

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Fix compile error

c52b627

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Fix style format

6ae0cf3

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Fix line ending for single line properties

f3132a5

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Fix integration tests with property type E_INT32

7f8e424

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Fix 'post' command help text

741d7c4

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

678098 self-requested a review May 15, 2024 11:31

alexander-e1off added 2 commits May 15, 2024 15:48

Fix spaces in string handling

db3c078

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Remove debug

0c3ce06

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

alexander-e1off changed the title ~~WIP: Enhance bmqtool to read poison pill message dumps~~ Enhance bmqtool to read poison pill message dumps May 15, 2024

alexander-e1off commented May 15, 2024

View reviewed changes

alexander-e1off changed the title ~~Enhance bmqtool to read poison pill message dumps~~ Enhance bmqtool to handle poison pill message dumps May 15, 2024

678098 requested changes May 15, 2024

View reviewed changes

678098 assigned alexander-e1off May 15, 2024

alexander-e1off marked this pull request as draft May 16, 2024 06:36

alexander-e1off added 2 commits May 17, 2024 12:14

Address review comments

090014a

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Add unit tests for bmqtool_inpututil

ed384c6

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

alexander-e1off added 4 commits May 24, 2024 15:29

Refactor to use new command load-post

a435e10

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Cleanup, rollback previous solution

2834b10

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Fix code style

be0b84e

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Fix code style

bd98443

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

alexander-e1off added 2 commits May 24, 2024 18:04

Cosmetic fixes

93f6078

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Merge branch 'main' into poisonpill

71c0585

alexander-e1off marked this pull request as ready for review May 24, 2024 16:00

Cosmetic fixes

9f51ac5

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

alexander-e1off assigned 678098 and unassigned alexander-e1off May 24, 2024

alexander-e1off added 3 commits May 27, 2024 14:15

Pass the blob buffer factory from the caller

2024f82

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Remove uneeded include

0bd73de

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Fix typos

3b99574

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

678098 reviewed May 28, 2024

View reviewed changes

alexander-e1off added 2 commits May 28, 2024 18:10

Address review comments

ce9e8b1

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>

Merge branch 'main' into poisonpill

c8aba48

678098 approved these changes May 28, 2024

View reviewed changes

alexander-e1off merged commit 0636f8f into bloomberg:main May 29, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance bmqtool to handle poison pill message dumps #288

Enhance bmqtool to handle poison pill message dumps #288

alexander-e1off commented May 14, 2024 •

edited

alexander-e1off May 15, 2024

678098 left a comment

678098 May 15, 2024

678098 May 15, 2024

678098 May 15, 2024

678098 May 15, 2024

678098 May 15, 2024

678098 May 15, 2024

678098 May 15, 2024

678098 May 15, 2024

678098 May 15, 2024

alexander-e1off commented May 21, 2024

alexander-e1off commented May 24, 2024 •

edited

678098 commented May 24, 2024

678098 left a comment

678098 May 28, 2024

678098 May 28, 2024

678098 May 28, 2024

678098 May 28, 2024

678098 May 28, 2024

678098 May 28, 2024

678098 May 28, 2024

678098 left a comment

	BSLA_MAYBE_UNUSED int result =
	BSLA_MAYBE_UNUSED const int result =

		@@ -149,11 +150,55 @@ void InputUtil::populateProperties(
		BSLS_ASSERT_SAFE(0 == result);

	/// Read hexdump from the specified `in` and write binary into th specified
	/// Read hexdump from the specified `in` and write binary into the specified

		<element name='uri' type='string'/>
		<element name='file' type='string'/>

	break; // stop at the end of hexdump (empty line)
	// stop at the end of hexdump (empty line)
	break; // BREAK

	/// Load message content from the specified `file` (created by
	/// Load message content from the specified `filePath` (created by


		BALL_LOG_INFO << "--> Loading and posting message: " << command;

		int rc;

	rc = d_session_p->post(eventBuilder.messageEvent());
	const int rc = d_session_p->post(eventBuilder.messageEvent());

Enhance bmqtool to handle poison pill message dumps #288

Enhance bmqtool to handle poison pill message dumps #288

Conversation

alexander-e1off commented May 14, 2024 • edited

Choose a reason for hiding this comment

678098 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

alexander-e1off commented May 21, 2024

alexander-e1off commented May 24, 2024 • edited

678098 commented May 24, 2024

678098 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

678098 left a comment

Choose a reason for hiding this comment

alexander-e1off commented May 14, 2024 •

edited

alexander-e1off commented May 24, 2024 •

edited