Skip to content

fix dead loop(#183)#189

Merged
2 commits merged intomicrosoft:mainfrom
ZhangShushu123:main
Dec 24, 2021
Merged

fix dead loop(#183)#189
2 commits merged intomicrosoft:mainfrom
ZhangShushu123:main

Conversation

@ZhangShushu123
Copy link
Contributor

fix dead loop(#183)

@ghost
Copy link

ghost commented Dec 18, 2021

CLA assistant check
All CLA requirements met.

@karianna karianna requested review from a user and dsgrieve December 20, 2021 14:47
@ZhangShushu123
Copy link
Contributor Author

When will it be merged into main branch?

@karianna
Copy link
Member

@ZhangShushu123 Thank you so much for this PR! Apologies for the delay. Most of the committers are off on holiday leave until the first week of January.

@ghost
Copy link

ghost commented Dec 24, 2021

Hi @ ZhangShushu123, Thank you for this patch and my apologies for the delay in addressing this. I did look at this as soon as it came in. I was battling getting 2.0.5 out and maven central wasn't cooperating and given the bits you were touching, I wanted to engage in a deeper review which I simply didn't have the cycles for. Now I have...

As an FYI, the issue (not with your patch) with this chunk of code is that the parsers have direct knowledge of the message bus which I feel is a violation of SRP (Single Responsibility Principle). In the implementation that we had prior to the modularization refactoring separated this behaviour into a message bus connector object that wrapped the parser. That refactoring is likely going to take some time.

The nice thing about your patch is that it addresses another issue and that is that all of the aggregators are registered irregardless of their applicability to the log being parsed. So, lets kill the dead loop code and deal with the other stuff later.

@ghost ghost merged commit 8cfddd2 into microsoft:main Dec 24, 2021
@ZhangShushu123
Copy link
Contributor Author

Thank you very much for taking the time to merge code. And I look forward to your next refactoring work.

This pull request was closed.
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