Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2200833004:
Fixing RedisRTC implementation and test (Closed)
|
Created:
4 years, 4 months ago by stephana Modified:
4 years, 4 months ago Reviewers:
dogben CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/buildbot@master Target Ref:
refs/heads/master Project:
skiabuildbot Visibility:
Public. |
DescriptionFixing RedisRTC implementation and test
This simplifies the ReadThroughCache implementation, improves
the test to test a more realistic use case and fixes to test to pass in the CQ.
BUG=skia:
Committed: https://skia.googlesource.com/buildbot/+/ecc4714f42638ef3bb944ace9df89125ff3c569e
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : Refactored readthrough cache to be simpler #Patch Set 4 : Removed dead code #
Total comments: 14
Patch Set 5 : Incorporated Feedback #
Total comments: 3
Messages
Total messages: 27 (21 generated)
The CQ bit was checked by stephana@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stephana@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stephana@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fixing readthrough test BUG=skia: ========== to ========== Fixing RedisRTC implementation and test This simplifies the ReadThroughCache implementation, improves the test to test a more realistic use case and fixes to test to pass in the CQ. BUG=skia: ==========
stephana@google.com changed reviewers: + benjaminwagner@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc.go File go/redisutil/rtc.go (right): https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc.go#new... go/redisutil/rtc.go:103: // queueSubChannel: fmt.Sprintf("__keyspace@%d__:%s", redisPool.db, queueKey), nit: delete https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc.go#new... go/redisutil/rtc.go:296: _, err := redis.Values(c.Do("BLPOP", r.workReadyKey, 1)) Seems like if the current program crashes after popping from workReady, the task won't be picked up by anyone, even after rebooting, until another task is enqueued. This might not be an issue, but I figured I would point it out. https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.go File go/redisutil/rtc_test.go (right): https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:18: N_TASKS = 1 Did you intend to test only one task? Seems like the higher number of tasks was the only reason we caught the issue with not closing. https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:35: // TODO (stephana): Re-enable this test once we have a way to cleanly shutdown delete? https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:46: Do you want assert.NoError here, and not return err below? https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:53: // codec := StringCodec{} nit: delete https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:95: assert.Equal(t, 1024*512+7, len(ret.([]byte))) nit: maybe add a comment to explain this value
The CQ bit was checked by stephana@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc.go File go/redisutil/rtc.go (right): https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc.go#new... go/redisutil/rtc.go:103: // queueSubChannel: fmt.Sprintf("__keyspace@%d__:%s", redisPool.db, queueKey), On 2016/08/08 21:18:24, Ben Wagner wrote: > nit: delete Done. https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc.go#new... go/redisutil/rtc.go:296: _, err := redis.Values(c.Do("BLPOP", r.workReadyKey, 1)) On 2016/08/08 21:18:24, Ben Wagner wrote: > Seems like if the current program crashes after popping from workReady, the task > won't be picked up by anyone, even after rebooting, until another task is > enqueued. > > This might not be an issue, but I figured I would point it out. True, I changed it so that if there the queue timed out, it will also return true, forcing the caller to poll the actual work queue. https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.go File go/redisutil/rtc_test.go (right): https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:18: N_TASKS = 1 On 2016/08/08 21:18:24, Ben Wagner wrote: > Did you intend to test only one task? Seems like the higher number of tasks was > the only reason we caught the issue with not closing. Good catch. That was for debugging. Changed to 1000. https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:35: // TODO (stephana): Re-enable this test once we have a way to cleanly shutdown On 2016/08/08 21:18:24, Ben Wagner wrote: > delete? Done. https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:46: On 2016/08/08 21:18:24, Ben Wagner wrote: > Do you want assert.NoError here, and not return err below? Done. https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:53: // codec := StringCodec{} On 2016/08/08 21:18:24, Ben Wagner wrote: > nit: delete Done. https://codereview.chromium.org/2200833004/diff/60001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:95: assert.Equal(t, 1024*512+7, len(ret.([]byte))) On 2016/08/08 21:18:24, Ben Wagner wrote: > nit: maybe add a comment to explain this value Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stephana@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from benjaminwagner@google.com Link to the patchset: https://codereview.chromium.org/2200833004/#ps80001 (title: "Incorporated Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fixing RedisRTC implementation and test This simplifies the ReadThroughCache implementation, improves the test to test a more realistic use case and fixes to test to pass in the CQ. BUG=skia: ========== to ========== Fixing RedisRTC implementation and test This simplifies the ReadThroughCache implementation, improves the test to test a more realistic use case and fixes to test to pass in the CQ. BUG=skia: Committed: https://skia.googlesource.com/buildbot/+/ecc4714f42638ef3bb944ace9df89125ff3c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/buildbot/+/ecc4714f42638ef3bb944ace9df89125ff3c...
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/2200833004/diff/80001/go/redisutil/rtc.go File go/redisutil/rtc.go (right): https://codereview.chromium.org/2200833004/diff/80001/go/redisutil/rtc.go#new... go/redisutil/rtc.go:298: // cause the caller to poll the queue. Why checkForWork() at all? Why not just poll the queue continuously? https://codereview.chromium.org/2200833004/diff/80001/go/redisutil/rtc_test.go File go/redisutil/rtc_test.go (right): https://codereview.chromium.org/2200833004/diff/80001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:59: // Wait for 2 seconds to make sure wait for work function times out "2 seconds" vs. "1 * time.Second" below https://codereview.chromium.org/2200833004/diff/80001/go/redisutil/rtc_test.g... go/redisutil/rtc_test.go:60: // at least once and queries the queueu directly. typo: "queueu" |
