ronantakizawa/codereview-bench huggingface dataset:
Repository: nsqio/nsq
File: nsqd/diskqueue.go (Go)

Code before:

		if d.readPos > 0 {
			_, err = d.readFile.Seek(d.readPos, 0)
			if err != nil {
				d.readFile.Close()
				d.readFile = nil
				return nil, err
			}
		}

		d.reader = bufio.NewReader(d.readFile)
	}

	err = binary.Read(d.reader, binary.BigEndian, &msgSize)
	if err != nil {
		d.readFile.Close()
		d.readFile = nil
		return nil, err
	}

	readBuf := make([]byte, msgSize)
	_, err = io.ReadFull(d.reader, readBuf)
	if err != nil {
		d.readFile.Close()
		d.readFile = nil
		return nil, err
	}

	totalBytes := int64(4 + msgSize)

	// we only advance next* because we have not yet sent this to consumers
	// (where readFileNum, readPos will actually be advanced)
	d.nextReadPos = d.readPos + totalBytes
	d.nextReadFileNum = d.readFileNum

	// TODO: each data file should embed the maxBytesPerFile
	// as the first 8 bytes (at creation time) ensuring that
	// the value can change without affecting runtime
	if d.nextReadPos > d.maxBytesPerFile {
		if d.readFile != nil {
			d.readFile.Close()
			d.readFile = nil
		}

		d.nextReadFileNum++
		d.nextReadPos = 0
	}

	return readBuf, nil
}


Review comment (suggestion):
msgSize could be > d.maxMsgSize because of either

a) a corrupt message, or
b) a person changed the flag of their maxMsgSize between nsqd restarts

If it's a corrupt message, then we'd skip forward in the file and then have no guarantee of what we're reading. If a person changed their maxMsgSize, they'd miss this message and be fine reading the next. I don't think it's too common to reduce MaxMsgSize between nsqd restarts other than when a person is trying to figure out parameters, and if they're fine with losing any large messages when tuning, I can't see why they wouldn't be fine losing an entire segment.

I favor discarding the file but I could go either way.


Code after:
		if err != nil {
			return nil, err
		}

		d.logf("DISKQUEUE(%s): readOne() opened %s", d.name, curFileName)

		if d.readPos > 0 {
			_, err = d.readFile.Seek(d.readPos, 0)
			if err != nil {
				d.readFile.Close()
				d.readFile = nil
				return nil, err
			}
		}

		d.reader = bufio.NewReader(d.readFile)
	}

	err = binary.Read(d.reader, binary.BigEndian, &msgSize)
	if err != nil {
		d.readFile.Close()
		d.readFile = nil
		return nil, err
	}

	if msgSize < d.minMsgSize || msgSize > d.maxMsgSize {
		// this file is corrupt and we have no reasonable guarantee on
		// where a new message should begin
		d.readFile.Close()
		d.readFile = nil
		return nil, fmt.Errorf("invalid message read size (%d)", msgSize)
	}

	readBuf := make([]byte, msgSize)
	_, err = io.ReadFull(d.reader, readBuf)
	if err != nil {
		d.readFile.Close()
		d.readFile = nil
		return nil, err
	}

	totalBytes := int64(4 + msgSize)

	// we only advance next* because we have not yet sent this to consumers
	// (where readFileNum, readPos will actually be advanced)
	d.nextReadPos = d.readPos + totalBytes
	d.nextReadFileNum = d.readFileNum

	// TODO: each data file should embed the maxBytesPerFile
	// as the first 8 bytes (at creation time) ensuring that
	// the value can change without affecting runtime
Repository: nsqio/nsq
File: nsqadmin/static/js/models/channel.js (JavaScript)

Code before:
var AppState = require('../app_state');
var Backbone = require('backbone');

var Channel = Backbone.Model.extend({
    idAttribute: 'name',

    constructor: function Channel() {
        Backbone.Model.prototype.constructor.apply(this, arguments);
    },

    url: function() {
        return AppState.url('/topics/' +
            encodeURIComponent(this.get('topic')) + '/' +
            encodeURIComponent(this.get('name')));
    },
    initialize: function() {
      this.on('change:clients', this.calculateHostnamePort);
    },
    calculateHostnamePort: function(m, v) {
      
      this.get("clients").forEach(function(v){
        var client_id = v["client_id"]; 
        var hostname = v["hostname"];
        var remote_address = v["remote_address"];
      
        // ignore client_id if it's duplicative
        if (client_id === hostname.split(".")[0] || client_id === hostname) {
          v["client_id"] = "";
        }

        var port = remote_address.split(":").pop()
        v['hostname_port'] =  hostname + ":" + port;
      })
    }
});

module.exports = Channel;


Review comment (suggestion):
I'd remove these arguments as they aren't used in the body of this method and can cause confusion with the `v` variable in the `forEach` method's iterator.


Code after:
var AppState = require('../app_state');
var Backbone = require('backbone');

var Channel = Backbone.Model.extend({
    idAttribute: 'name',

    constructor: function Channel() {
        Backbone.Model.prototype.constructor.apply(this, arguments);
    },

    url: function() {
        return AppState.url('/topics/' +
            encodeURIComponent(this.get('topic')) + '/' +
            encodeURIComponent(this.get('name')));
    },

    initialize: function() {
      this.on('change:clients', this.calculateHostnamePort);
    },

    calculateHostnamePort: function() {
        this.get("clients").forEach(function(client){
            var client_id = client["client_id"];
            var hostname = client["hostname"];
            var remote_address = client["remote_address"];
  
            // ignore client_id if it's duplicative
            if (client_id === hostname.split(".")[0] || client_id === hostname) {
                client["client_id"] = "";
            }

            var port = remote_address.split(":").pop()
            client['hostname_port'] =  hostname + ":" + port;
        })
    }
});

module.exports = Channel;
Repository: nsqio/nsq
File: nsqadmin/static/js/app_state.js (JavaScript)

Code before:
var Backbone = require('backbone');

var AppState = Backbone.Model.extend({
    defaults: function() {
        return {
            'VERSION': VERSION,
            'GRAPHITE_URL': GRAPHITE_URL,
            'GRAPH_ENABLED': GRAPH_ENABLED,
            'STATSD_INTERVAL': STATSD_INTERVAL,
            'STATSD_PREFIX': STATSD_PREFIX,
            'NSQLOOKUPD': NSQLOOKUPD,
            'graph_interval': '2h'
        };
    },

    initialize: function() {
        this.on('change:graph_interval', function(model, v) {
            localStorage.setItem('graph_interval', v);
        });

        var qp = _.object(_.compact(_.map(window.location.search.slice(1).split('&'),
            function(item) { if (item) { return item.split('='); } })));
        var interval = qp['t'] || localStorage.getItem('graph_interval') || 'off';
        this.set('graph_interval', interval);
    },

    url: function(url) {
        return '/api' + url;
    }
});

var appState = new AppState();

window.AppState = appState;

module.exports = appState;


Review comment (suggestion):
@jehiah note: there's no real parsing or validation, the code that reads this value requires it to be in the form `/[0-9]+h/`.

Do we need to support other durations?

(I should probably add a little validation here...)


Code after:
var Backbone = require('backbone');
var _ = require('underscore');

var AppState = Backbone.Model.extend({
    defaults: function() {
        return {
            'VERSION': VERSION,
            'GRAPHITE_URL': GRAPHITE_URL,
            'GRAPH_ENABLED': GRAPH_ENABLED,
            'STATSD_INTERVAL': STATSD_INTERVAL,
            'STATSD_PREFIX': STATSD_PREFIX,
            'NSQLOOKUPD': NSQLOOKUPD,
            'graph_interval': '2h'
        };
    },

    initialize: function() {
        this.on('change:graph_interval', function(model, v) {
            localStorage.setItem('graph_interval', v);
        });

        var qp = _.object(_.compact(_.map(window.location.search.slice(1).split('&'),
            function(item) { if (item) { return item.split('='); } })));

        var def = this.get('GRAPH_ENABLED') ? '2h' : 'off';
        var interval = qp['t'] || localStorage.getItem('graph_interval') || def;
        this.set('graph_interval', interval);
    },

    url: function(url) {
        return '/api' + url;
    }
});

var appState = new AppState();

window.AppState = appState;

module.exports = appState;
Repository: nsqio/nsq
File: nsqadmin/static/js/app_state.js (JavaScript)

Code before:
var Backbone = require('backbone');

var AppState = Backbone.Model.extend({
    defaults: function() {
        return {
            'VERSION': VERSION,
            'GRAPHITE_URL': GRAPHITE_URL,
            'GRAPH_ENABLED': GRAPH_ENABLED,
            'STATSD_INTERVAL': STATSD_INTERVAL,
            'STATSD_PREFIX': STATSD_PREFIX,
            'NSQLOOKUPD': NSQLOOKUPD,
            'graph_interval': '2h'
        };
    },

    initialize: function() {
        this.on('change:graph_interval', function(model, v) {
            localStorage.setItem('graph_interval', v);
        });

        var qp = _.object(_.compact(_.map(window.location.search.slice(1).split('&'),
            function(item) { if (item) { return item.split('='); } })));
        var interval = qp['t'] || localStorage.getItem('graph_interval') || 'off';
        this.set('graph_interval', interval);
    },

    url: function(url) {
        return '/api' + url;
    }
});

var appState = new AppState();

window.AppState = appState;

module.exports = appState;


Review comment (suggestion):
Let's just support them all and not care? `(s|seconds?|min|minutes?|h|hours?|d|days?|w|weeks?|mon|months?|y|years?)` seems to be the valid list (despite the [docs](http://graphite.readthedocs.org/en/latest/render_api.html#from-until) only showing a subset)


Code after:
var Backbone = require('backbone');
var _ = require('underscore');

var AppState = Backbone.Model.extend({
    defaults: function() {
        return {
            'VERSION': VERSION,
            'GRAPHITE_URL': GRAPHITE_URL,
            'GRAPH_ENABLED': GRAPH_ENABLED,
            'STATSD_INTERVAL': STATSD_INTERVAL,
            'STATSD_PREFIX': STATSD_PREFIX,
            'NSQLOOKUPD': NSQLOOKUPD,
            'graph_interval': '2h'
        };
    },

    initialize: function() {
        this.on('change:graph_interval', function(model, v) {
            localStorage.setItem('graph_interval', v);
        });

        var qp = _.object(_.compact(_.map(window.location.search.slice(1).split('&'),
            function(item) { if (item) { return item.split('='); } })));

        var def = this.get('GRAPH_ENABLED') ? '2h' : 'off';
        var interval = qp['t'] || localStorage.getItem('graph_interval') || def;
        this.set('graph_interval', interval);
    },

    url: function(url) {
        return '/api' + url;
    }
});

var appState = new AppState();

window.AppState = appState;

module.exports = appState;
Repository: nsqio/nsq
File: nsqadmin/static/js/app_state.js (JavaScript)

Code before:
var Backbone = require('backbone');

var AppState = Backbone.Model.extend({
    defaults: function() {
        return {
            'VERSION': VERSION,
            'GRAPHITE_URL': GRAPHITE_URL,
            'GRAPH_ENABLED': GRAPH_ENABLED,
            'STATSD_INTERVAL': STATSD_INTERVAL,
            'STATSD_PREFIX': STATSD_PREFIX,
            'NSQLOOKUPD': NSQLOOKUPD,
            'graph_interval': '2h'
        };
    },

    initialize: function() {
        this.on('change:graph_interval', function(model, v) {
            localStorage.setItem('graph_interval', v);
        });

        var qp = _.object(_.compact(_.map(window.location.search.slice(1).split('&'),
            function(item) { if (item) { return item.split('='); } })));
        var interval = qp['t'] || localStorage.getItem('graph_interval') || 'off';
        this.set('graph_interval', interval);
    },

    url: function(url) {
        return '/api' + url;
    }
});

var appState = new AppState();

window.AppState = appState;

module.exports = appState;


Review comment (suggestion):
also, while here, can we default to `2h` when you have graphite configured? (we used to do that see [here](https://github.com/nsqio/nsq/pull/323/files#diff-c0e97967285692126a91af4fdca200daL130) )


Code after:
var Backbone = require('backbone');
var _ = require('underscore');

var AppState = Backbone.Model.extend({
    defaults: function() {
        return {
            'VERSION': VERSION,
            'GRAPHITE_URL': GRAPHITE_URL,
            'GRAPH_ENABLED': GRAPH_ENABLED,
            'STATSD_INTERVAL': STATSD_INTERVAL,
            'STATSD_PREFIX': STATSD_PREFIX,
            'NSQLOOKUPD': NSQLOOKUPD,
            'graph_interval': '2h'
        };
    },

    initialize: function() {
        this.on('change:graph_interval', function(model, v) {
            localStorage.setItem('graph_interval', v);
        });

        var qp = _.object(_.compact(_.map(window.location.search.slice(1).split('&'),
            function(item) { if (item) { return item.split('='); } })));

        var def = this.get('GRAPH_ENABLED') ? '2h' : 'off';
        var interval = qp['t'] || localStorage.getItem('graph_interval') || def;
        this.set('graph_interval', interval);
    },

    url: function(url) {
        return '/api' + url;
    }
});

var appState = new AppState();

window.AppState = appState;

module.exports = appState;
Repository: nsqio/nsq
File: nsqadmin/static/js/app_state.js (JavaScript)

Code before:
var Backbone = require('backbone');

var AppState = Backbone.Model.extend({
    defaults: function() {
        return {
            'VERSION': VERSION,
            'GRAPHITE_URL': GRAPHITE_URL,
            'GRAPH_ENABLED': GRAPH_ENABLED,
            'STATSD_INTERVAL': STATSD_INTERVAL,
            'STATSD_PREFIX': STATSD_PREFIX,
            'NSQLOOKUPD': NSQLOOKUPD,
            'graph_interval': '2h'
        };
    },

    initialize: function() {
        this.on('change:graph_interval', function(model, v) {
            localStorage.setItem('graph_interval', v);
        });

        var qp = _.object(_.compact(_.map(window.location.search.slice(1).split('&'),
            function(item) { if (item) { return item.split('='); } })));
        var interval = qp['t'] || localStorage.getItem('graph_interval') || 'off';
        this.set('graph_interval', interval);
    },

    url: function(url) {
        return '/api' + url;
    }
});

var appState = new AppState();

window.AppState = appState;

module.exports = appState;


Review comment (suggestion):
I think that should work. Here it is without underscore (and in one loop):

``` javascript
var queryParams = window.location.search.slice(1).split('&').reduce(function(params, qStr) {
    if (qStr) {
        var pieces = qStr.split('=');
        params[pieces[0]] = pieces[1];
    }
    return params;
}, {});
```

Also, I think `Array.prototype.map` is now supported by all major browsers (http://caniuse.com/#feat=es5), so there's also this:

``` javascript
var qp = _.object(_.compact(window.location.search.slice(1).split('&').map(function(item) {
    return item ? item.split('=') : null;
})));
```


Code after:
var Backbone = require('backbone');
var _ = require('underscore');

var AppState = Backbone.Model.extend({
    defaults: function() {
        return {
            'VERSION': VERSION,
            'GRAPHITE_URL': GRAPHITE_URL,
            'GRAPH_ENABLED': GRAPH_ENABLED,
            'STATSD_INTERVAL': STATSD_INTERVAL,
            'STATSD_PREFIX': STATSD_PREFIX,
            'NSQLOOKUPD': NSQLOOKUPD,
            'graph_interval': '2h'
        };
    },

    initialize: function() {
        this.on('change:graph_interval', function(model, v) {
            localStorage.setItem('graph_interval', v);
        });

        var qp = _.object(_.compact(_.map(window.location.search.slice(1).split('&'),
            function(item) { if (item) { return item.split('='); } })));

        var def = this.get('GRAPH_ENABLED') ? '2h' : 'off';
        var interval = qp['t'] || localStorage.getItem('graph_interval') || def;
        this.set('graph_interval', interval);
    },

    url: function(url) {
        return '/api' + url;
    }
});

var appState = new AppState();

window.AppState = appState;

module.exports = appState;
Repository: nsqio/nsq
File: nsqd/protocol_v2.go (Go)

Code before:
		}

		// trim the '\n'
		line = line[:len(line)-1]
		// optionally trim the '\r'
		if len(line) > 0 && line[len(line)-1] == '\r' {
			line = line[:len(line)-1]
		}
		params := bytes.Split(line, separatorBytes)

		if p.ctx.nsqd.getOpts().Verbose {
			p.ctx.nsqd.logf("PROTOCOL(V2): [%s] %s", client, params)
		}

		response, err := p.Exec(client, params)
		if err != nil {
			ctx := ""
			if parentErr := err.(protocol.ChildErr).Parent(); parentErr != nil {
				ctx = " - " + parentErr.Error()
			}
			p.ctx.nsqd.logf("ERROR: [%s] - %s%s", client, err, ctx)

			sendErr := p.Send(client, frameTypeError, []byte(err.Error()))
			if sendErr != nil {
				break
			}

			// errors of type FatalClientErr should forceably close the connection
			if _, ok := err.(*protocol.FatalClientErr); ok {
				break
			}
			continue
		}

		if response != nil {
			err = p.Send(client, frameTypeResponse, response)
			if err != nil {
				err = fmt.Errorf("failed to send response - %s", err)
				break
			}
		}
	}

	p.ctx.nsqd.logf("PROTOCOL(V2): [%s] exiting ioloop", client)
	conn.Close()
	close(client.ExitChan)
	if client.Channel != nil {
		client.Channel.RemoveClient(client.ID)
	}

	return err

Review comment (suggestion):
Probably not. `Exec` is going to return a `FatalClientErr` and we don't want to overwrite it on our way out.


Code after:
		}

		// trim the '\n'
		line = line[:len(line)-1]
		// optionally trim the '\r'
		if len(line) > 0 && line[len(line)-1] == '\r' {
			line = line[:len(line)-1]
		}
		params := bytes.Split(line, separatorBytes)

		if p.ctx.nsqd.getOpts().Verbose {
			p.ctx.nsqd.logf("PROTOCOL(V2): [%s] %s", client, params)
		}

		var response []byte
		response, err = p.Exec(client, params)
		if err != nil {
			ctx := ""
			if parentErr := err.(protocol.ChildErr).Parent(); parentErr != nil {
				ctx = " - " + parentErr.Error()
			}
			p.ctx.nsqd.logf("ERROR: [%s] - %s%s", client, err, ctx)

			sendErr := p.Send(client, frameTypeError, []byte(err.Error()))
			if sendErr != nil {
				p.ctx.nsqd.logf("ERROR: [%s] - %s%s", client, sendErr, ctx)
				break
			}

			// errors of type FatalClientErr should forceably close the connection
			if _, ok := err.(*protocol.FatalClientErr); ok {
				break
			}
			continue
		}

		if response != nil {
			err = p.Send(client, frameTypeResponse, response)
			if err != nil {
				err = fmt.Errorf("failed to send response - %s", err)
				break
			}
		}
	}

	p.ctx.nsqd.logf("PROTOCOL(V2): [%s] exiting ioloop", client)
	conn.Close()
	close(client.ExitChan)
	if client.Channel != nil {
		client.Channel.RemoveClient(client.ID)
	}
Repository: nsqio/nsq
File: nsqd/protocol_v2.go (Go)

Code before:
		}

		// trim the '\n'
		line = line[:len(line)-1]
		// optionally trim the '\r'
		if len(line) > 0 && line[len(line)-1] == '\r' {
			line = line[:len(line)-1]
		}
		params := bytes.Split(line, separatorBytes)

		if p.ctx.nsqd.getOpts().Verbose {
			p.ctx.nsqd.logf("PROTOCOL(V2): [%s] %s", client, params)
		}

		response, err := p.Exec(client, params)
		if err != nil {
			ctx := ""
			if parentErr := err.(protocol.ChildErr).Parent(); parentErr != nil {
				ctx = " - " + parentErr.Error()
			}
			p.ctx.nsqd.logf("ERROR: [%s] - %s%s", client, err, ctx)

			sendErr := p.Send(client, frameTypeError, []byte(err.Error()))
			if sendErr != nil {
				break
			}

			// errors of type FatalClientErr should forceably close the connection
			if _, ok := err.(*protocol.FatalClientErr); ok {
				break
			}
			continue
		}

		if response != nil {
			err = p.Send(client, frameTypeResponse, response)
			if err != nil {
				err = fmt.Errorf("failed to send response - %s", err)
				break
			}
		}
	}

	p.ctx.nsqd.logf("PROTOCOL(V2): [%s] exiting ioloop", client)
	conn.Close()
	close(client.ExitChan)
	if client.Channel != nil {
		client.Channel.RemoveClient(client.ID)
	}

	return err

Review comment (bug):
Let me clarify without editing - if `Exec` returned a `FatalClientError` we probably want the returned error to be the one which determined `IOLoop` is going to exit. In this case, `sendErr` is secondary.

If `Exec` returned another type of error it may not have necessarily exited, but we are now that `p.Send` has failed. In this case, if we want to return the error which caused the loop to exit it would be `sendErr`. A few if statements and we could have this behavior.

I think, please correct me if I'm wrong, but at this time all errors returned by `Exec` are `FatalClientErr`.


Code after:
		}

		// trim the '\n'
		line = line[:len(line)-1]
		// optionally trim the '\r'
		if len(line) > 0 && line[len(line)-1] == '\r' {
			line = line[:len(line)-1]
		}
		params := bytes.Split(line, separatorBytes)

		if p.ctx.nsqd.getOpts().Verbose {
			p.ctx.nsqd.logf("PROTOCOL(V2): [%s] %s", client, params)
		}

		var response []byte
		response, err = p.Exec(client, params)
		if err != nil {
			ctx := ""
			if parentErr := err.(protocol.ChildErr).Parent(); parentErr != nil {
				ctx = " - " + parentErr.Error()
			}
			p.ctx.nsqd.logf("ERROR: [%s] - %s%s", client, err, ctx)

			sendErr := p.Send(client, frameTypeError, []byte(err.Error()))
			if sendErr != nil {
				p.ctx.nsqd.logf("ERROR: [%s] - %s%s", client, sendErr, ctx)
				break
			}

			// errors of type FatalClientErr should forceably close the connection
			if _, ok := err.(*protocol.FatalClientErr); ok {
				break
			}
			continue
		}

		if response != nil {
			err = p.Send(client, frameTypeResponse, response)
			if err != nil {
				err = fmt.Errorf("failed to send response - %s", err)
				break
			}
		}
	}

	p.ctx.nsqd.logf("PROTOCOL(V2): [%s] exiting ioloop", client)
	conn.Close()
	close(client.ExitChan)
	if client.Channel != nil {
		client.Channel.RemoveClient(client.ID)
	}