sort out context issues #205
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
jschwinger233
commented
Jun 3, 2020
•
jschwinger233
commented
- 检查所有调用 rpc 层和 store 层对 context 的使用情况, 只有 CreateLock(key string, ttl Duration) 没有 context, 但是研究了一下它是利用了 etcd client 的 context 作为 session context, 也还行吧, 如果 client 中断, 虽然 CreateLock 不受影响, 但是接下来 lock.Lock(ctx) 会中断, defer 里的 Unlock 也会 close session, 应该没啥问题.
- 把所有直接使用 context.Background() 都改成 context.WithTimeout(context.Background(), config.GlobalTimeout)
- 把两个 realloc 和 replace rpc 调用改成了使用 stream.Context()
- realloc 在 VirutalUpdateResource 之前允许 client 打断, 之后不行, 因为没有回滚机制
- replace 在 remove 老容器之前的操作都是允许 client 打断, 之后就不行了, 因为删容器不可回滚
|
测试挂了.. |
CMGS
reviewed
Jun 3, 2020
cluster/calcium/lambda.go
Outdated
| defer func() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), c.config.GlobalTimeout) | ||
| defer cancel() | ||
| c.doRemoveContainerSync(ctx, []string{message.ContainerID}) |
There was a problem hiding this comment.
我觉得这里的 timeout 处理得放到这个 doRemoveContainerSync 里面
There was a problem hiding this comment.
我放到更下层了, 分别在 Container type 的 Stop / Remove / ... 方法里, 和 Mercury 的 Get / Put / ... 方法里,
There was a problem hiding this comment.
types.Container 带一个 timeout 参数蛮奇怪的，如果放方法给 Stop/Remove ... 多一个传参呢
There was a problem hiding this comment.
改到上一层 control container 里统一做了
CMGS
reviewed
Jun 3, 2020
CMGS
reviewed
Jun 3, 2020
CMGS
reviewed
Jun 3, 2020
cluster/calcium/lambda.go
Outdated
| // 不能让 context 作祟 | ||
| backgroundCtx := context.Background() | ||
| createChan, err := c.CreateContainer(backgroundCtx, opts) | ||
| log.Info("in creating") |
CMGS
reviewed
Jun 4, 2020
metrics/handler.go
Outdated
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| pods, err := cluster.ListPods(ctx) | ||
| pods, err := cluster.ListPods(context.Background()) |
CMGS
reviewed
Jun 4, 2020
CMGS
reviewed
Jun 4, 2020
CMGS
reviewed
Jun 4, 2020
CMGS
reviewed
Jun 4, 2020
cluster/calcium/lock.go
Outdated
| @@ -16,21 +16,25 @@ func (c *Calcium) doLock(ctx context.Context, name string, timeout time.Duration | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| ctx, cancel := context.WithTimeout(ctx, c.config.GlobalTimeout) | |||
| defer cancel() | |||
| if err = lock.Lock(ctx); err != nil { | |||
There was a problem hiding this comment.
顺便改这里啊 return lock, lock.Lock(ctx)
|
LGTM |
atlas-comstock
pushed a commit
to atlas-comstock/core
that referenced
this pull request
Jun 11, 2020
* all background context have global timeout * encapsulate transaction * let reallac and replace use client context * better transaction encap * add ctx timeout in Container and Mercury * make test pass * add timeout in control container * engine interface decorator to add timeout * exempt interactive requests from timeout
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.