Skip to content

Conversation

@darcydai
Copy link
Contributor

@darcydai darcydai commented Sep 1, 2021

Fix

  • Add a unit test to verify that the fix works.

  • Explain briefly why the bug exists and how to fix it.

  • Update the CHANGES log.

This is a big change for the lettuce plugin

Lettuce is an async Redis client base on netty, and wide use in our company.
Recently I found almost every lettuce span has the time elapsed less than 1ms and has a long gap between two spans. After reading the lettuce source code, I found the plugin some bug.
because lettuce is an async framework base on netty, old interceptor named 'RedisChannelWriterInterceptor' stop span in after method can not record the time elapsed, it should be close in 'RedisCommand#complete' or 'RedisCommand#completeExceptionally' or 'RedisCommand#cancel' called when Redis server response
lettuce plugin record nothing when lettuce work on reactive mode (project reactor)
record Redis peer in skywalking dynamic field can simplify

The new plugin can work with Redis standalone and Redis Sentinel and Redis cluster(old version can not). Redis cluster and Redis sentinel need to use docker-compose. I do not know how to make scenarios test in this two cluster model

@wu-sheng wu-sheng added the enhancement New feature or request label Sep 1, 2021
@wu-sheng wu-sheng requested a review from zhaoyuguang September 1, 2021 23:58
@wu-sheng wu-sheng added this to the 8.8.0 milestone Sep 1, 2021
@wu-sheng wu-sheng added the plugin label Sep 2, 2021
@wu-sheng
Copy link
Member

wu-sheng commented Sep 2, 2021

The new plugin can work with Redis standalone and Redis Sentinel and Redis cluster(old version can not). Redis cluster and Redis sentinel need to use docker-compose. I do not know how to make scenarios test in this two cluster model

It is fine to skip that. If you want, it is better to update support list doc to mention all these.

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM. @darcydai Do you need to update the document to mention this?

The new plugin can work with Redis standalone and Redis Sentinel and Redis cluster(old version can not). Redis cluster and Redis sentinel need to use docker-compose. I do not know how to make scenarios test in this two cluster model

@darcydai
Copy link
Contributor Author

darcydai commented Sep 2, 2021

LGTM. @darcydai Do you need to update the document to mention this?

The new plugin can work with Redis standalone and Redis Sentinel and Redis cluster(old version can not). Redis cluster and Redis sentinel need to use docker-compose. I do not know how to make scenarios test in this two cluster model

where should I write this?

@wu-sheng
Copy link
Member

wu-sheng commented Sep 2, 2021

@wu-sheng wu-sheng merged commit c872adc into apache:main Sep 2, 2021
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.

2 participants