-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Refactor cbdb_log to use vfprintf #506
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hiiii, @ruhuang2001 welcome!🎊 Thanks for taking the effort to make our project better! 🙌 Keep making such awesome contributions!
@ruhuang2001 thanks for your contribution! |
Please add test cases. |
OK, I understand, but I am not very familiar with the whole system. What are the requirements or standards for the test cases, or can you give me some suggestions? Thanks a lot! |
Always we should add test in regress/isolation/isolation2/tap test etc when fix code, but this change is related with log output which is not convenient to add test case. Maybe it's ok to merge this without test case while it's better to consider other ways to add some test? What's more, we should remove unnecessary code diff. |
Hi, thanks for your contribution. The codes overall looks good to me, and you could do better. |
For writing a good commit message, welcome to take this template for reference. For more tips on Git/GitHub and contribution guide, see here: https://cloudberrydb.org/contribute#find-a-way-to-contribute. Hope can help you! |
Thanks for suggestions and review!
I found that it might be a Windows newline character issue. When I edited in the web editor, autocrlf was enabled by default. I fix in the latest commit.
In the latest changes, I tried to use fprintf to avoid using a buffer.
When I was addressing the * issue in the previous commit, I accidentally used force push, which seems to have corrupted the commit. Should I create a new branch to submit the changes ? I am so sorry 😢 |
You could use git rebase to arrange your codes to the top, refer to git manual. |
cde3a76
to
e1de740
Compare
Ok, now all my own commits are merged into one committ. |
@avamingli 'src/fe_utils/log.c' this file is not found in postgres, is it added by cbdb? |
Yes.
I'm not sure. Hi, @jiaqizho do you have any context about this? |
Using vfprintf to avoid unnecessary buffer
yes, it added by yifan, used to provider a log util for the fe process. The reason why it is built as frontend mode and exposed is that we don't want use the same log path with the |
fix #89
Change logs
Describe your change clearly, including what problem is being solved or what feature is being added.
If it has some breaking backward or forward compatibility, please clary.
Why are the changes needed?
Describe why the changes are necessary.
Does this PR introduce any user-facing change?
If yes, please clarify the previous behavior and the change this PR proposes.
How was this patch tested?
Please detail how the changes were tested, including manual tests and any relevant unit or integration tests.
Contributor's Checklist
Here are some reminders and checklists before/when submitting your pull request, please check them:
make installcheck
make -C src/test installcheck-cbdb-parallel
cloudberrydb/dev
team for review and approval when your PR is ready🥳