Skip to content
This repository was archived by the owner on Oct 24, 2023. It is now read-only.

fix: protect the slice for append operation in multi-thread scenario#198

Merged
tariq1890 merged 1 commit intoAzure:masterfrom
andyliuliming:master_fix_race_issue
Jan 2, 2019
Merged

fix: protect the slice for append operation in multi-thread scenario#198
tariq1890 merged 1 commit intoAzure:masterfrom
andyliuliming:master_fix_race_issue

Conversation

@andyliuliming
Copy link
Member

for more details about the issue please take a look at this:
https://medium.com/@cep21/gos-append-is-not-always-thread-safe-a3034db7975

@andyliuliming
Copy link
Member Author

@thomas1206

@andyliuliming
Copy link
Member Author

Hi @mboersma , I think there's some issue on the circleci golang lint.
it ask us to do these changes for the style:
"github.com/mattn/go-colorable" => colorable "github.com/mattn/go-colorable"

and all the api version model:
"github.com/Azure/aks-engine/pkg/api/v20160330" =>v20160330 "github.com/Azure/aks-engine/pkg/api/v20160330"

will leave it to you whether we need changes like that..

@zhongyi-zhang
Copy link

Agree. This is something we should take care of. The underlying issue could be hard to track and repro.

@andyliuliming
Copy link
Member Author

/assign @mboersma

@mboersma
Copy link
Member

Thanks @andyliuliming. We may have a fix for the goimports check in #204. I'll get that fixed and look at your PR today.

@tariq1890
Copy link
Contributor

@andyliuliming Could you rebase this PR?

@andyliuliming
Copy link
Member Author

@andyliuliming Could you rebase this PR?

rebased, thanks :)

tariq1890
tariq1890 previously approved these changes Dec 28, 2018
Copy link
Contributor

@tariq1890 tariq1890 left a comment

Choose a reason for hiding this comment

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

Approved with minor comments.

@mboersma mboersma changed the title We should protect the slice for append operation in multi thread scenario fix: protect the slice for append operation in multi-thread scenario Dec 31, 2018
@andyliuliming
Copy link
Member Author

@mboersma could you please help merge? Thanks.

@tariq1890
Copy link
Contributor

@andyliuliming can you address my last comment? I can merge after that

@codecov
Copy link

codecov bot commented Jan 2, 2019

Codecov Report

Merging #198 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #198      +/-   ##
=========================================
+ Coverage   53.18%   53.2%   +0.02%     
=========================================
  Files          95      95              
  Lines       14228   14235       +7     
=========================================
+ Hits         7567    7574       +7     
  Misses       5995    5995              
  Partials      666     666

@codecov
Copy link

codecov bot commented Jan 2, 2019

Codecov Report

Merging #198 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #198      +/-   ##
=========================================
+ Coverage   53.18%   53.2%   +0.02%     
=========================================
  Files          95      95              
  Lines       14228   14235       +7     
=========================================
+ Hits         7567    7574       +7     
  Misses       5995    5995              
  Partials      666     666

@andyliuliming andyliuliming force-pushed the master_fix_race_issue branch from a6b5ee8 to 8470a42 Compare January 2, 2019 00:51
@andyliuliming
Copy link
Member Author

@andyliuliming can you address my last comment? I can merge after that

done, thanks very much :)

Copy link
Contributor

@tariq1890 tariq1890 left a comment

Choose a reason for hiding this comment

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

/lgtm

@acs-bot
Copy link

acs-bot commented Jan 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyliuliming, tariq1890

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tariq1890 tariq1890 merged commit 5cadc11 into Azure:master Jan 2, 2019
juhacket pushed a commit to juhacket/aks-engine that referenced this pull request Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants