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

Try update value if node already exists #2314

Merged
merged 3 commits into from
Jun 3, 2023

Conversation

chickenlj
Copy link
Contributor

No description provided.

@AlexStocks
Copy link
Contributor

@chickenlj please fix the ci failure.

@@ -344,8 +344,7 @@ func (l *ZkEventListener) listenDirEvent(conf *common.URL, zkRootPath string, li
failTimes = MaxFailTimes
}

err = perrors.Cause(err)
if !strings.Contains(err.Error(), "node does not exist") { // ignore if node not exist
if !perrors.Is(err, zk.ErrNoNode) { // ignore if node not exist
Copy link
Contributor

@AlexStocks AlexStocks May 16, 2023

Choose a reason for hiding this comment

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

@chickenlj 这个地方我有异议,这个地方不需要修改啊,原来的没啥问题。你修改后,会不会遗漏一些case就不晓得了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

实际上,上面的字符串关键字 node does not exist 也是从 ErrNoNode 定义中取的,因此两者能检查的范围应该是一致的。

ErrNoNode = errors.New("zk: node does not exist")

perrors.Is 文档说是会嵌套往下检查,理论上与应该没啥问题。

@sonarcloud
Copy link

sonarcloud bot commented May 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@codecov-commenter
Copy link

Codecov Report

Merging #2314 (cbcfdb4) into main (cbcfdb4) will not change coverage.
The diff coverage is n/a.

❗ Current head cbcfdb4 differs from pull request most recent head 825a21f. Consider uploading reports for the commit 825a21f to get more accurate results

@@           Coverage Diff           @@
##             main    #2314   +/-   ##
=======================================
  Coverage   44.19%   44.19%           
=======================================
  Files         294      294           
  Lines       17867    17867           
=======================================
  Hits         7897     7897           
  Misses       9127     9127           
  Partials      843      843           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AlexStocks AlexStocks merged commit 9d087ed into apache:main Jun 3, 2023
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.

5 participants