Skip to content

Add thrift plugin support thrift TMultiplexedProcessor.#22

Merged
wu-sheng merged 7 commits into
apache:mainfrom
zifeihan:zifeihan
Sep 14, 2021
Merged

Add thrift plugin support thrift TMultiplexedProcessor.#22
wu-sheng merged 7 commits into
apache:mainfrom
zifeihan:zifeihan

Conversation

@zifeihan
Copy link
Copy Markdown
Member

Fix thrift plugin does not support thrift TMultiplexedProcessor, so NPE is triggered

  • Add a unit test to verify that the fix works.
    It has been tested through the demo submitted by the user. I have been too busy recently. I will submit the test scenario in two days.

  • Explain briefly why the bug exists and how to fix it.
    Check 7571 to understand the cause of the bug and how to fix it.

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #7571 .

  • Update the CHANGES log.

@zifeihan zifeihan requested review from dmsolr and wu-sheng September 12, 2021 15:05
@wu-sheng wu-sheng added bug Something isn't working plugin labels Sep 12, 2021
@wu-sheng wu-sheng added this to the 8.8.0 milestone Sep 12, 2021
@dmsolr
Copy link
Copy Markdown
Member

dmsolr commented Sep 12, 2021

I think this is an enhancement. :)
We don't implement all Processors.

@dmsolr
Copy link
Copy Markdown
Member

dmsolr commented Sep 12, 2021

Hi @lzdujing Please check it if you have time.

@dmsolr
Copy link
Copy Markdown
Member

dmsolr commented Sep 12, 2021

@zifeihan maybe we need to list of Processors in docs which we have enhanced, WDYT?

@zifeihan
Copy link
Copy Markdown
Member Author

zifeihan commented Sep 12, 2021

@zifeihan maybe we need to list of Processors in docs which we have enhanced, WDYT?

I think it is necessary. Currently thrift only has 3 processors, and we currently support all of them.

@wu-sheng wu-sheng added enhancement New feature or request and removed bug Something isn't working labels Sep 13, 2021
@dmsolr
Copy link
Copy Markdown
Member

dmsolr commented Sep 13, 2021

@zifeihan maybe we need to list of Processors in docs which we have enhanced, WDYT?

I think it is necessary. Currently thrift only has 3 processors, and we currently support all of them.

Actually, not only these are 3. There are also many wrapped processors.

@zifeihan
Copy link
Copy Markdown
Member Author

zifeihan commented Sep 13, 2021

image
The object here is new, so it does not go through ServerInProtocolWrapper, so special judgment is required in org.apache.skywalking.apm.plugin.thrift.TBaseProcessorInterceptor

Copy link
Copy Markdown
Member

@dmsolr dmsolr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @zifeihan told me who is too busy to complete docs.

@wu-sheng
Copy link
Copy Markdown
Member

LGTM, @zifeihan told me who is too busy to complete docs.

Hi, we don't have typically documents to host all logic of plugins, they are too complex to describe. AFAIK, only very few plugin added README.md in the root of plugin source codes only. @dmsolr If you want to, welcome to send a pull request later.

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

Labels

enhancement New feature or request plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants