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

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

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

678098 · 2025-02-28T20:42:04Z

Hi @ariraein, I am busy this week and can review or help on the next. For now, I can launch the CI to check if the PR builds

ariraein · 2025-02-28T20:44:18Z

Hi @678098 Thank you. I tried building locally and it was successful. If the PR fails, I will try my best to take care of it.

ariraein · 2025-03-15T14:33:49Z

hi @678098 when you have a chance can you please let me know know what am I doing wrong in the process?

1 - built successfully locally
2 - clang-format everything in root blazingmq directory
3 - add all modified files (11)
4 - DCO check steps

git checkout "refactor-aligned-printer returns ~70 files, of which only 11 were modified.
5 - I go through Refactor aligned printer #605 (comment) steps
6 - 5 checks fails, 4 skipped checks

678098 · 2025-03-20T14:42:17Z

Hi @ariraein

1 - built successfully locally
I think the default local build steps don't include building bmqstoragetool. This is why you can build the rest (bmqbrkr, bmqtool), but the CI fails on building bmqstoragetool:

/home/runner/work/blazingmq/blazingmq/src/applications/bmqstoragetool/m_bmqstoragetool_searchresult.cpp:1599:20: error: ‘d_queueDeleteRecordsMap’ was not declared in this scope
 1599 |         printer << d_queueDeleteRecordsMap[queueKey];
      |                    ^~~~~~~~~~~~~~~~~~~~~~~

2 - clang-format everything in root blazingmq directory
If you run git clang-format, it will only format the changed files added/updated (but not committed yet) to git. If you had formatting problem in the previous commit, you can either run clang-format on problematic files clang-format -i <path_to_file>, or alternatively you can revert the last commit and re-apply clang-format on file diff:

# check your current branch and commits
git status
git log

# if you are in your branch and the last commit has formatting problem, revert it but keep changes
git reset --soft HEAD~1

# apply clang-format on the files from the reverted commit
git clang-format

# add the updated formatted files and commit again
git add .
git commit -s -m "<commit message>"

# if you are sure that the changes are good you can update your remote fork with the new version of the branch
# git push -f is always a risk, you need to be extra careful not to overwrite good changes
git push -f origin <your branch>

678098 · 2025-03-20T14:56:16Z

src/applications/bmqstoragetool/m_bmqstoragetool_journalfileprocessor.t.cpp

+                      "    Num Delete Records    : 5";
+
+    bsl::string res(resultStream.str(), bmqtst::TestHelperUtil::allocator());
+    ASSERT(res.starts_with(expectedStream.str()));


A general comment, in the PR's description there are much more additions than deletions: +662 −72
It looks suspicious because you can expect the numbers to be pretty close if you are doing code refactor. If you look closely at the changed files, many of them have additions only. This means that the PR is adding new code rather than changing the existing one. I think the reason for this is the conflict resolution process when the code moved to another places in the main branch. You should probably search for the place where the code moved and apply changes there.
If you see places where the code was just added without modification of other code, it's probably a place where the conflict resolution went wrong

…pdate usage Signed-off-by: Ari Raein <[email protected]>

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

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

ariraein force-pushed the refactor-aligned-printer branch from bce40cf to 66c46e9 Compare February 28, 2025 00:21

678098 self-assigned this Feb 28, 2025

ariraein force-pushed the refactor-aligned-printer branch 2 times, most recently from 01f226e to 4604e20 Compare March 13, 2025 21:22

678098 reviewed Mar 20, 2025

View reviewed changes

678098 requested a review from alexander-e1off March 27, 2025 12:14

Refactor AlignedPrinter: change from const char* to bsl::string and u…

86a7a1a

…pdate usage Signed-off-by: Ari Raein <[email protected]>

ariraein force-pushed the refactor-aligned-printer branch from 4604e20 to 86a7a1a Compare April 13, 2025 02:53

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

678098 commented Feb 28, 2025

ariraein commented Feb 28, 2025

ariraein commented Mar 15, 2025

678098 commented Mar 20, 2025

678098 Mar 20, 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 28, 2025

ariraein commented Feb 28, 2025

ariraein commented Mar 15, 2025

678098 commented Mar 20, 2025

Choose a reason for hiding this comment

678098 commented Feb 21, 2025 •

edited

Loading