Skip to content

Conversation

@cjappl
Copy link
Owner

@cjappl cjappl commented Dec 24, 2024

No description provided.

@cjappl
Copy link
Owner Author

cjappl commented Dec 24, 2024

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

atsushieno added a commit to atsushieno/uapmd that referenced this pull request Dec 25, 2024
@atsushieno
Copy link
Contributor

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 swap_queues branch in my pet project, and it seems working well so far (maybe I need to audio for a while too). I will let you know if I found any problem regarding these changes.

@cjappl
Copy link
Owner Author

cjappl commented Dec 25, 2024

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

EXPECT_EQ(strlen(buffer.data()), this->maxMessageLength - 1);
};
EXPECT_EQ(truncatedLogger.PrintAndClearLogQueue(InspectLogMessage), 1);
EXPECT_EQ(this->logger.PrintAndClearLogQueue(InspectLogMessage), 2);
Copy link
Owner Author

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

cjappl added a commit that referenced this pull request Dec 28, 2024
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.
@cjappl cjappl merged commit 36381f9 into main Jan 13, 2025
5 checks passed
@cjappl cjappl deleted the swap_queues branch January 13, 2025 10:03
@cjappl
Copy link
Owner Author

cjappl commented Jan 13, 2025

@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.

@atsushieno
Copy link
Contributor

Awesome! Thank you for all the continuous improvements over the branch. I once switched to swap_queues branch and it's been working fine. But I was not on the latest commits, so I will keep my eyes on it for a while, now that I switched back to your main branch.

@cjappl
Copy link
Owner Author

cjappl commented Mar 2, 2025

@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

@atsushieno
Copy link
Contributor

@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.

@cjappl
Copy link
Owner Author

cjappl commented Mar 2, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants