-
Notifications
You must be signed in to change notification settings - Fork 6
Changed default queue to farbot, added Multi and Single Writer variants #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Requesting a review from @atsushieno - no rush at all. I need to sit on this and revisit it with new eyes in a week or so. Seems like all my tests are happy with both new variants |
|
Thank you so much for taking time to make it, it seems perfect to me! Now I have switched the reference to rtlog-cpp to |
|
Sounds good. Thanks for your feedback and prod to do this! As I said in the previous comment, I'm going to let this sit for a week or so and try to come back with fresh eyes. Let me know if you have any notes in the meantime. Cheers |
test/test_rtlog.cpp
Outdated
| EXPECT_EQ(strlen(buffer.data()), this->maxMessageLength - 1); | ||
| }; | ||
| EXPECT_EQ(truncatedLogger.PrintAndClearLogQueue(InspectLogMessage), 1); | ||
| EXPECT_EQ(this->logger.PrintAndClearLogQueue(InspectLogMessage), 2); |
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.
Question for future me to figure out: Why did this need to change to 2?
More specifically, why did it return 1 in the first place? It should have enqueued both the truncated and non truncated messages (2). This indicates that there was a bug in the original version.
Need to investigate more before possibly changing some behavior. I think this new way is correct, I just want to understand more
Add a timeout in case tests overrun. In #33 there will be a requirement to have powers of two queues, get that set up for everlog.
|
@atsushieno - this has been merged to main. Will you please change your project to test off that commit, and if you find no problems (in a month or so) let me know and I can cut an official release with the changes in it. Thanks again for suggesting the change and helping implement it. |
|
Awesome! Thank you for all the continuous improvements over the branch. I once switched to |
|
@atsushieno - has this change been working for you? Any notes or feedback, or do you feel it is stable enough to be released in an "official version" of rtlog? If things have been going fine for you I will cut a tag and release it soon |
|
@cjappl sorry but I have been busy on non-C++ projects these weeks so I cannot give any reliable report on that. I found no problem but that does not mean any credibility. |
|
Thanks for the update! I'll consider if I want to do a release or not.
Good luck on your other projects!
…On Sun, Mar 2, 2025 at 11:30 AM, Atsushi Eno ***@***.***(mailto:On Sun, Mar 2, 2025 at 11:30 AM, Atsushi Eno <<a href=)> wrote:
***@***.***(https://github.com/cjappl) sorry but I have been busy on non-C++ projects these weeks so I cannot give any reliable report on that. I found no problem but that does not mean any credibility.
—
Reply to this email directly, [view it on GitHub](#33 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXY437DW7QWQB5XJEHDD2SJUMNAVCNFSM6AAAAABUFDM7L6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJSGUZDCNJUGU).
You are receiving this because you were mentioned.Message ID: ***@***.***>
[atsushieno] atsushieno left a comment [(cjappl/rtlog-cpp#33)](#33 (comment))
***@***.***(https://github.com/cjappl) sorry but I have been busy on non-C++ projects these weeks so I cannot give any reliable report on that. I found no problem but that does not mean any credibility.
—
Reply to this email directly, [view it on GitHub](#33 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXY437DW7QWQB5XJEHDD2SJUMNAVCNFSM6AAAAABUFDM7L6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJSGUZDCNJUGU).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
No description provided.