Refactor aligned printer #605

ariraein · 2025-02-11T02:42:30Z

Issue number of the reported bug or feature request: #

Describe your changes
A clear and concise description of the changes you have made.

Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.

Additional context
Add any other context about your contribution here.

678098

Added a review notes
CI DCO check fails for the second commit in the PR. All commits need to be DCO-signed to pass this. The instructions how it can be simply fixed are here: https://github.com/bloomberg/blazingmq/blob/main/CONTRIBUTING.md#dco-check
CI formatter check fails, need to run clang-format on source files before commiting, or alternatively you can use CI output to make fixes: https://github.com/bloomberg/blazingmq/actions/runs/13254786590/job/37026607247?pr=605
Build fails, might be related to review notes

678098 · 2025-02-11T13:31:16Z

.gitignore

+docker/sanitizers/
+lib64/
+src/groups/mqb/mqbc/mqbc_clusterutil.t.cpp
+src/groups/mqb/mqbc/mqbc_clusterutil.t.cpp


Among these newly added items:

Some are not expected in a normal workflow:

.bash_history .bde_tools/ lib64/

Some are legit project-related files or folders and there is no reason to ignore them:

docker/sanitizers/ src/groups/mqb/mqbc/mqbc_clusterutil.t.cpp src/groups/mqb/mqbc/mqbc_clusterutil.t.cpp

Let's revert changes in this file

678098 · 2025-02-11T13:33:00Z

src/applications/bmqstoragetool/m_bmqstoragetool_journalfileprocessor.t.cpp

@@ -1219,7 +1219,7 @@ static void test14_summaryTest()
    fields.push_back("Number of partially confirmed messages");
    fields.push_back("Number of confirmed messages");
    fields.push_back("Number of outstanding messages");
-    bmqu::AlignedPrinter printer(expectedStream, &fields);
+    bmqu::AlignedPrinter printer(expectedStream, fields);


Also need to change type a few lines above:
bsl::vector<const char*> fields -> bsl::vector<bsl::string> fields

678098 · 2025-02-11T13:33:19Z

src/applications/bmqstoragetool/m_bmqstoragetool_journalfileprocessor.t.cpp

@@ -2108,7 +2108,7 @@ static void test24_summaryWithQueueDetailsTest()
    fields.push_back("Number of partially confirmed messages");
    fields.push_back("Number of confirmed messages");
    fields.push_back("Number of outstanding messages");
-    bmqu::AlignedPrinter printer(expectedStream, &fields);
+    bmqu::AlignedPrinter printer(expectedStream, fields);


Also need to change type a few lines above:
bsl::vector<const char*> fields -> bsl::vector<bsl::string> fields

678098 · 2025-02-11T13:33:25Z

src/applications/bmqstoragetool/m_bmqstoragetool_recordprinter.cpp

@@ -71,7 +71,7 @@ void printRecord(bsl::ostream&                  stream,
    fields.push_back("GUID");
    fields.push_back("Crc32c");

-    bmqu::AlignedPrinter printer(stream, &fields);
+    bmqu::AlignedPrinter printer(stream, fields);


Also need to change type, and do the same in other places in this file:
bsl::vector<const char*> fields -> bsl::vector<bsl::string> fields

678098 · 2025-02-11T13:33:52Z

src/applications/bmqstoragetool/m_bmqstoragetool_recordprinter.cpp

@@ -155,7 +155,7 @@ void printRecord(bsl::ostream&                  stream,
        }
    }

-    bmqu::AlignedPrinter printer(stream, &fields);
+    bmqu::AlignedPrinter printer(stream, fields);


Also need to change type above:
bsl::vector<const char*> fields -> bsl::vector<bsl::string> fields

678098 · 2025-02-11T13:35:42Z

src/applications/bmqstoragetool/m_bmqstoragetool_searchresult.cpp

@@ -81,7 +81,7 @@ void printJournalFileMeta(bsl::ostream&              ostream,
    fields.push_back("SyncPoint DataFileOffset (DWORDS)");
    fields.push_back("SyncPoint QlistFileOffset (WORDS)");

-    bmqu::AlignedPrinter printer(ostream, &fields);
+    bmqu::AlignedPrinter printer(ostream, fields);


Also need to change type, and do the same in other places in this file:
bsl::vector<const char*> fields -> bsl::vector<bsl::string> fields

678098 · 2025-02-11T13:36:01Z

src/applications/bmqtool/m_bmqtool_storageinspector.cpp

@@ -474,7 +474,7 @@ void StorageInspector::processCommand(
            fields.push_back("SyncPoint DataFileOffset (DWORDS)");
            fields.push_back("SyncPoint QlistFileOffset (WORDS)");

-            bmqu::AlignedPrinter printer(BALL_LOG_OUTPUT_STREAM, &fields);
+            bmqu::AlignedPrinter printer(BALL_LOG_OUTPUT_STREAM, fields);


Also need to change type above, and do the same in other places in this file:
bsl::vector<const char*> fields -> bsl::vector<bsl::string> fields

678098 · 2025-02-11T13:40:46Z

src/groups/mqb/mqbs/mqbs_filestoreprotocolprinter.cpp

    it.loadAppIds(&appIdLenPairs);
    it.loadAppIdHashes(&appIdHashes);


Also need to updates methods loadAppIds and loadAppIdHashes to use bsl::string:

blazingmq/src/groups/mqb/mqbs/mqbs_qlistfileiterator.h

Line 158 in 89c1228

void loadAppIds(bsl::vector<AppIdLengthPair>* appIds) const;

blazingmq/src/groups/mqb/mqbs/mqbs_qlistfileiterator.h

Line 160 in 89c1228

void loadAppIdHashes(bsl::vector<const char*>* appIdHashes) const;

And this typedef:

blazingmq/src/groups/mqb/mqbs/mqbs_qlistfileiterator.h

Line 65 in 89c1228

typedef bsl::pair<const char*, unsigned int> AppIdLengthPair;

Hi Evgeny. Do you agree with the below?

typedef bsl::pair<bsl::string, unsigned int> AppIdLengthPair;

void loadAppIds(bsl::vector<AppIdLengthPair>& appIds) const;

void loadAppIdHashes(bsl::vector<bsl::string>& appIdHashes) const;

Hi @ariraein

typedef bsl::pair<bsl::string, unsigned int> AppIdLengthPair;

This change is good.

void loadAppIds(bsl::vector& appIds) const;

void loadAppIdHashes(bsl::vectorbsl::string& appIdHashes) const;

For these 2 cases, we still have to pass output argument by pointer according to BDE coding standard we use:

https://bloomberg.github.io/bde/knowledge_base/coding_standards.html

So it should be:

void loadAppIds(bsl::vector<AppIdLengthPair> *appIds) const;

void loadAppIdHashes(bsl::vector<bsl::string> *appIdHashes) const;

ariraein · 2025-02-19T04:17:21Z

Hi Evgeny. Sorry for the delay. The code changes/updates are done. The only problem is I'm trying to build locally before committing, so to cause no issues here, but so far having issues building on MacOS. Will take care of it by tomorrow. Thanks, Ari.

678098 · 2025-02-21T08:08:38Z

Hi @ariraein, thank you for fixing DCO. It seems that the files that you modify in this PR were modified by other people, and it causes merge conflicts. Can you solve them?

To start solving merge conflicts, you need to have the latest branch you want to merge to, in this case it's the base repository's main. You can update metadata for this version of main with these commands:

# probably you have your fork of the repo configured, you can also add the base repo as a remote:
git remote add upstream https://github.com/bloomberg/blazingmq.git
git fetch upstream

After this, you can start rebasing:

# make sure you are in the feature branch
git checkout refactor-aligned-printer

# you can make a backup of your branch before resolving conflicts just to be safe
git checkout -b refactor-aligned-printer-backup

# you have created a backup and also switched branch, now you can go back to the branch you want to fix:
git checkout refactor-aligned-printer

# this command will try to move commits in your branch so it starts from the latest main from the base repo:
git rebase upstream/main

git rebase will complain that it cannot perform rebase because there are conflicts. It will also output the list of files with conflicts. You will need to go to these files and apply changes to resolve them.

You can also check that the resolved version builds and after this you can continue git rebase:

# add files that had conflicts fixed
git add .

# check that you haven't added files that are not needed
git status

# finish rebase, the command should complete
git rebase --continue

# now you can update the version of the branch in remote (this PR)
git push -f origin refactor-aligned-printer

Hope these notes will be helpful. If you have any problems, don't hesitate to reach out

Signed-off-by: Ari Raein <aliraein1@gmail.com>

ariraein · 2025-02-22T04:10:48Z

Hi @678098 Thank you so much for the explanation and help.

ariraein requested a review from a team as a code owner February 11, 2025 02:42

678098 requested changes Feb 11, 2025

View reviewed changes

ariraein force-pushed the refactor-aligned-printer branch from c7e40fc to b1a7de7 Compare February 21, 2025 04:37

678098 mentioned this pull request Feb 21, 2025

Feat[bmqstoragetool]: Add unit tests for m_bmqstoragetool::Printer implementations #621

Merged

Refactor aligned printer-Retry

bce40cf

Signed-off-by: Ari Raein <aliraein1@gmail.com>

ariraein force-pushed the refactor-aligned-printer branch from b1a7de7 to bce40cf Compare February 22, 2025 04:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor aligned printer #605

Refactor aligned printer #605

ariraein commented Feb 11, 2025

678098 left a comment

678098 Feb 11, 2025

678098 Feb 11, 2025

678098 Feb 11, 2025

678098 Feb 11, 2025

678098 Feb 11, 2025

678098 Feb 11, 2025

678098 Feb 11, 2025

678098 Feb 11, 2025

ariraein Feb 13, 2025

678098 Feb 13, 2025

ariraein commented Feb 19, 2025

678098 commented Feb 21, 2025 •

edited

Loading

ariraein commented Feb 22, 2025

		it.loadAppIds(&appIdLenPairs);
		it.loadAppIdHashes(&appIdHashes);

Refactor aligned printer #605

Are you sure you want to change the base?

Refactor aligned printer #605

Conversation

ariraein commented Feb 11, 2025

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

Choose a reason for hiding this comment

ariraein commented Feb 19, 2025

678098 commented Feb 21, 2025 • edited Loading

ariraein commented Feb 22, 2025

678098 commented Feb 21, 2025 •

edited

Loading