blazingmq icon indicating copy to clipboard operation
blazingmq copied to clipboard

Refactor aligned printer

Open ariraein opened this issue 1 year ago • 11 comments

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.

ariraein avatar Feb 11 '25 02:02 ariraein

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.

ariraein avatar Feb 19 '25 04:02 ariraein

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

678098 avatar Feb 21 '25 08:02 678098

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

ariraein avatar Feb 22 '25 04:02 ariraein

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

678098 avatar Feb 28 '25 20:02 678098

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 avatar Feb 28 '25 20:02 ariraein

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 https://github.com/bloomberg/blazingmq/pull/605#issuecomment-2673858727 steps 6 - 5 checks fails, 4 skipped checks

ariraein avatar Mar 15 '25 14:03 ariraein

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 avatar Mar 20 '25 14:03 678098

Hi @ariraein Ari, I 've started to review, and noticed that changes from PR #665 aren't merged properly (I would guess due to merge conflicts?). Please check that all changes from PR #665 merged correctly. Also please check that CI is passed, I see there are still some build errors. Thanks!

alexander-e1off avatar Apr 17 '25 10:04 alexander-e1off

Hi @ariraein Ari, I 've started to review, and noticed that changes from PR #665 aren't merged properly (I would guess due to merge conflicts?). Please check that all changes from PR #665 merged correctly. Also please check that CI is passed, I see there are still some build errors. Thanks!

Hi @alexander-e1off , Done. Please let me know if you see it merging PR #665 correctly this time. Thanks,

ariraein avatar Apr 23 '25 18:04 ariraein

Hi @ariraein, thanks for your update! Hmm...I still see some issues of PR #665 merging in src/applications/bmqstoragetool/m_bmqstoragetool_printer.cpp file. I added comments at corresponding places in the file. Also, I see that CI is still failing - there are formatting check and build errors. Please check this too. Thanks!

alexander-e1off avatar Apr 24 '25 07:04 alexander-e1off

Hi @alexander-e1off please ignore my last message/build. working on remaining errors.

ariraein avatar May 05 '25 22:05 ariraein