Merge lp:~dave-cheney/juju-core/122-fix-lp-1301706 into lp:~go-bot/juju-core/trunk

Proposed by Dave Cheney on 2014-04-03
Status: Rejected
Rejected by: William Reade on 2014-04-03
Proposed branch: lp:~dave-cheney/juju-core/122-fix-lp-1301706
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 20 lines (+3/-3)
1 file modified
state/watcher/watcher.go (+3/-3)
To merge this branch: bzr merge lp:~dave-cheney/juju-core/122-fix-lp-1301706
Reviewer Review Type Date Requested Status
Juju Engineering 2014-04-03 Pending
Review via email: mp+213971@code.launchpad.net

Description of the change

Update LP 1301706

Whie investigating LP 1301706 I noticed that there were a few interface conversions which did not panic if the data coming back from mongo was corrupted. I think that this is wrong and the three conversions I've changed should panic if their do not match the expected type.

I hope this will also give some clarity to the underlying issue which is panicing as if a nil was passed to the filter func.

https://codereview.appspot.com/82910044/

To post a comment you must log in.
Dave Cheney (dave-cheney) wrote :

Reviewers: mp+213971_code.launchpad.net,

Message:
Please take a look.

Description:
Update LP 1301706

Whie investigating LP 1301706 I noticed that there were a few interface
conversions which did not panic if the data coming back from mongo was
corrupted. I think that this is wrong and the three conversions I've
changed should panic if their do not match the expected type.

I hope this will also give some clarity to the underlying issue which is
panicing as if a nil was passed to the filter func.

https://code.launchpad.net/~dave-cheney/juju-core/122-fix-lp-1301706/+merge/213971

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/82910044/

Affected files (+5, -3 lines):
   A [revision details]
   M state/watcher/watcher.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140403043051-b5x0yssr90gt3toe
+New revision: <email address hidden>

Index: state/watcher/watcher.go
=== modified file 'state/watcher/watcher.go'
--- state/watcher/watcher.go 2013-09-13 14:48:13 +0000
+++ state/watcher/watcher.go 2014-04-03 05:19:50 +0000
@@ -371,13 +371,13 @@
    for _, c := range entry[1:] {
     // See txn's Runner.ChangeLog for the structure of log entries.
     var d, r []interface{}
- dr, _ := c.Value.(bson.D)
+ dr := c.Value.(bson.D)
     for _, item := range dr {
      switch item.Name {
      case "d":
- d, _ = item.Value.([]interface{})
+ d = item.Value.([]interface{})
      case "r":
- r, _ = item.Value.([]interface{})
+ r = item.Value.([]interface{})
      }
     }
     if len(d) == 0 || len(d) != len(r) {

William Reade (fwereade) wrote :

I'm ambivalent about this. It's nice to stop when we encounter corrupted
data, but it's not so nice add new ways to panic the API server. Guy
says not lgtm on those grounds -- i think I'd rather log a suitably
terrifying error and keep going despite the degraded functionality.

Counterargument?

https://codereview.appspot.com/82910044/

Dave Cheney (dave-cheney) wrote :

Fair, but please review LP 1301076, we're already panicing, and Kapil
has to demo this next week

On Thu, Apr 3, 2014 at 7:42 PM, William Reade
<email address hidden> wrote:
> I'm ambivalent about this. It's nice to stop when we encounter corrupted
> data, but it's not so nice add new ways to panic the API server. Guy
> says not lgtm on those grounds -- i think I'd rather log a suitably
> terrifying error and keep going despite the degraded functionality.
>
> Counterargument?
>
> https://codereview.appspot.com/82910044/
>
> --
> https://code.launchpad.net/~dave-cheney/juju-core/122-fix-lp-1301706/+merge/213971
> You are the owner of lp:~dave-cheney/juju-core/122-fix-lp-1301706.

William Reade (fwereade) wrote :

Rejecting this approach -- AFAICT it just moves the panic around. I think the immediate don't-panic response is to log-and-continue an unexpected type, in the filter func in state/watcher.go:213; and the *correct* response is to refactor that watcher code to let us kill that watcher in response to weird data. Then that watcher dies, and we get errors feeding back to the affected clients; and the rest of the system can continue to function.

To be clear: I'd accept a short-term CL that did the former, resolving the panic, alongside a tech-debt bug to fix up the watchers to let the error propagate to the Right Place.

Unmerged revisions

2554. By Dave Cheney on 2014-04-03

Panic if values are not of the expected type

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/watcher/watcher.go'
2--- state/watcher/watcher.go 2013-09-13 14:48:13 +0000
3+++ state/watcher/watcher.go 2014-04-03 05:23:32 +0000
4@@ -371,13 +371,13 @@
5 for _, c := range entry[1:] {
6 // See txn's Runner.ChangeLog for the structure of log entries.
7 var d, r []interface{}
8- dr, _ := c.Value.(bson.D)
9+ dr := c.Value.(bson.D)
10 for _, item := range dr {
11 switch item.Name {
12 case "d":
13- d, _ = item.Value.([]interface{})
14+ d = item.Value.([]interface{})
15 case "r":
16- r, _ = item.Value.([]interface{})
17+ r = item.Value.([]interface{})
18 }
19 }
20 if len(d) == 0 || len(d) != len(r) {

Subscribers

People subscribed via source and target branches

to status/vote changes: