Skip to content
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

issue of zk push-empty protection #6162

Open
1 task
laywin opened this issue Dec 15, 2023 · 3 comments · May be fixed by #6206
Open
1 task

issue of zk push-empty protection #6162

laywin opened this issue Dec 15, 2023 · 3 comments · May be fixed by #6206
Assignees

Comments

@laywin
Copy link
Contributor

laywin commented Dec 15, 2023

  • I have searched the issues of this repository and believe that this is not a duplicate.

Ⅰ. Issue Description

zk not take into account the situation of push-empty protection
zk客户端实现没有考虑到空推保护的情况
image

Ⅱ. Describe what happened

If there is an exception, please attach the exception trace:

Just paste your stack trace here!

Ⅲ. Describe what you expected to happen

Ⅳ. How to reproduce it (as minimally and precisely as possible)

  1. xxx
  2. xxx
  3. xxx

Minimal yet complete reproducer code (or URL to code):

Ⅴ. Anything else we need to know?

Ⅵ. Environment:

  • JDK version(e.g. java -version):
  • Seata client/server version:
  • Database version:
  • OS(e.g. uname -a):
  • Others:
@funky-eyes
Copy link
Contributor

我怀疑这是一个bug,作者可能想的是node如果被删了最好把对应的cluster也删掉,避免浪费内存空间.但是else if中又对instaces做了判空处理,防止推空,所以这是一个矛盾的产物.我建议修复方案

  1. 当事务分组对应的cluster被切换时,做延迟删除,延迟删除可以有效保证如果在短时间内cluster切换回来,可以立即开始工作,而不需要又从zookeeper中读取一次. 也能满足原作者的目的,并且延迟删除将对应的listener也可以一并清除
  2. 将监听逻辑改为,如果当前group对应的cluster还是自身watch的node,那么就防推空,如果不是就允许推空.
    如果社区有更好的建议欢迎在这一起讨论

I suspect this is a bug. The author may be thinking that if node is deleted, it is best to delete the corresponding cluster to avoid wasting memory space. But in else if, instaces is judged empty to prevent empty, so this is a contradictory product. I suggest a fix

  1. When the cluster corresponding to the transaction group is switched, do delayed deletion. Delayed deletion can effectively ensure that if the cluster switches back in a short time, it can start working immediately without reading from the zookeeper again. It can also meet the purpose of the original author, and delayed deletion will also clear the corresponding listener together
  2. Change the listening logic to, if the cluster corresponding to the current group is still the node of its own watch, then anti-push empty, if not, allow push empty.
    If the community has better suggestions, welcome to discuss them together.

@slievrly
Copy link
Member

我怀疑这是一个bug,作者可能想的是node如果被删了最好把对应的cluster也删掉,避免浪费内存空间.但是else if中又对instaces做了判空处理,防止推空,所以这是一个矛盾的产物.我建议修复方案

  1. 当事务分组对应的cluster被切换时,做延迟删除,延迟删除可以有效保证如果在短时间内cluster切换回来,可以立即开始工作,而不需要又从zookeeper中读取一次. 也能满足原作者的目的,并且延迟删除将对应的listener也可以一并清除
  2. 将监听逻辑改为,如果当前group对应的cluster还是自身watch的node,那么就防推空,如果不是就允许推空.
    如果社区有更好的建议欢迎在这一起讨论

I suspect this is a bug. The author may be thinking that if node is deleted, it is best to delete the corresponding cluster to avoid wasting memory space. But in else if, instaces is judged empty to prevent empty, so this is a contradictory product. I suggest a fix

  1. When the cluster corresponding to the transaction group is switched, do delayed deletion. Delayed deletion can effectively ensure that if the cluster switches back in a short time, it can start working immediately without reading from the zookeeper again. It can also meet the purpose of the original author, and delayed deletion will also clear the corresponding listener together
  2. Change the listening logic to, if the cluster corresponding to the current group is still the node of its own watch, then anti-push empty, if not, allow push empty.
    If the community has better suggestions, welcome to discuss them together.

I agree with this plan. Regarding the original cluster list, I think we can consider not deleting it for the time being, as the actual data volume it occupies is quite small.

@laywin
Copy link
Contributor Author

laywin commented Dec 17, 2023

我怀疑这是一个bug,作者可能想的是node如果被删了最好把对应的cluster也删掉,避免浪费内存空间.但是else if中又对instaces做了判空处理,防止推空,所以这是一个矛盾的产物.我建议修复方案

  1. 当事务分组对应的cluster被切换时,做延迟删除,延迟删除可以有效保证如果在短时间内cluster切换回来,可以立即开始工作,而不需要又从zookeeper中读取一次. 也能满足原作者的目的,并且延迟删除将对应的listener也可以一并清除
  2. 将监听逻辑改为,如果当前group对应的cluster还是自身watch的node,那么就防推空,如果不是就允许推空.
    如果社区有更好的建议欢迎在这一起讨论

I suspect this is a bug. The author may be thinking that if node is deleted, it is best to delete the corresponding cluster to avoid wasting memory space. But in else if, instaces is judged empty to prevent empty, so this is a contradictory product. I suggest a fix

  1. When the cluster corresponding to the transaction group is switched, do delayed deletion. Delayed deletion can effectively ensure that if the cluster switches back in a short time, it can start working immediately without reading from the zookeeper again. It can also meet the purpose of the original author, and delayed deletion will also clear the corresponding listener together
  2. Change the listening logic to, if the cluster corresponding to the current group is still the node of its own watch, then anti-push empty, if not, allow push empty.
    If the community has better suggestions, welcome to discuss them together.

理解一下,处理逻辑和 #6164 redis 处理的差不多的,移除的时候需要 check 一下 回调集群 如果和 当前正在使用的集群 不一致,才能允许空推移除, 并且需要移除listener, 取消订阅

Understand that the processing logic is similar to that of #6164 redis processing. When removing, you need to check the callback cluster. If it is inconsistent with the cluster currently in use, empty push removal can be allowed. , and need to remove the listener and unsubscribe

@laywin laywin linked a pull request Dec 25, 2023 that will close this issue
1 task
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 a pull request may close this issue.

3 participants