Commit 79301b83 authored by ale's avatar ale

golint and documentation fixes

Also removed some unnecessary duplication of code from net/http.
parent 2bea900a
......@@ -5,6 +5,8 @@ import (
"fmt"
"log"
_ "net/http/pprof"
"git.autistici.org/ale/autoradio"
"git.autistici.org/ale/autoradio/fe"
"git.autistici.org/ale/autoradio/instrumentation"
......@@ -36,10 +38,10 @@ func main() {
client := autoradio.NewClient(autoradio.NewEtcdClient(false))
dnsRed := fe.NewDnsRedirector(client, *domain, util.IPListWithDefault(*publicIps, "127.0.0.1"), dnsTtl)
dnsRed.Run(fmt.Sprintf(":%d", *dnsPort))
dnsRed := fe.NewDNSRedirector(client, *domain, util.IPListWithDefault(*publicIps, "127.0.0.1"), dnsTtl)
dnsRed.Start(fmt.Sprintf(":%d", *dnsPort))
red, err := fe.NewHttpRedirector(client, *domain, *lbPolicy, *staticDir, *templateDir)
red, err := fe.NewHTTPRedirector(client, *domain, *lbPolicy, *staticDir, *templateDir)
if err != nil {
log.Fatal(err)
}
......
......@@ -7,7 +7,7 @@ import (
)
// Filter a list of IP addresses by protocol.
func filterIpByProto(ips []net.IP, v6 bool) []net.IP {
func filterIPByProto(ips []net.IP, v6 bool) []net.IP {
var candidates []net.IP
for _, ip := range ips {
isIPv6 := (ip.To4() == nil)
......@@ -19,8 +19,8 @@ func filterIpByProto(ips []net.IP, v6 bool) []net.IP {
}
// Pick a random IP for the specified proto.
func randomIpByProto(ips []net.IP, v6 bool) net.IP {
candidates := filterIpByProto(ips, v6)
func randomIPByProto(ips []net.IP, v6 bool) net.IP {
candidates := filterIPByProto(ips, v6)
if len(candidates) > 0 {
return candidates[rand.Intn(len(candidates))]
}
......
......@@ -15,7 +15,7 @@ import (
var (
// Max number of results for an A query.
maxResults = 4
maxResults = 3
// The names that we are serving. Currently, all services are
// mapped to all the active nodes in the cluster.
......@@ -30,8 +30,8 @@ var (
dnsTargetStats = instrumentation.NewCounter("dns.target")
)
// DNS server.
type DnsRedirector struct {
// DNSRedirector sends clients to backends using DNS.
type DNSRedirector struct {
client *autoradio.Client
origin string
originNumParts int
......@@ -40,9 +40,9 @@ type DnsRedirector struct {
soa dns.RR
}
// NewDnsRedirector returns a DNS server for the given origin and
// NewDNSRedirector returns a DNS server for the given origin and
// publicIp. The A records served will have the specified ttl.
func NewDnsRedirector(client *autoradio.Client, origin string, publicIps []net.IP, ttl int) *DnsRedirector {
func NewDNSRedirector(client *autoradio.Client, origin string, publicIps []net.IP, ttl int) *DNSRedirector {
if !strings.HasSuffix(origin, ".") {
origin += "."
}
......@@ -64,7 +64,7 @@ func NewDnsRedirector(client *autoradio.Client, origin string, publicIps []net.I
Minttl: uint32(ttl),
}
return &DnsRedirector{
return &DNSRedirector{
client: client,
origin: origin,
originNumParts: len(dns.SplitDomainName(origin)),
......@@ -104,26 +104,15 @@ func ednsFromRequest(req, m *dns.Msg) {
return
}
func (d *DnsRedirector) withOrigin(name string) string {
func (d *DNSRedirector) withOrigin(name string) string {
if name == "" {
return d.origin
}
return name + "." + d.origin
}
// Create an A RR for a specific IP.
func (d *DnsRedirector) recordForIp(name string, ip net.IP, v6 bool) dns.RR {
if v6 {
return &dns.AAAA{
Hdr: dns.RR_Header{
Name: d.withOrigin(name),
Rrtype: dns.TypeAAAA,
Class: dns.ClassINET,
Ttl: uint32(d.ttl),
},
AAAA: ip,
}
}
// Create an A resource record.
func (d *DNSRedirector) newA(name string, ip net.IP) dns.RR {
return &dns.A{
Hdr: dns.RR_Header{
Name: d.withOrigin(name),
......@@ -135,8 +124,21 @@ func (d *DnsRedirector) recordForIp(name string, ip net.IP, v6 bool) dns.RR {
}
}
// Create an AAAA resource record.
func (d *DNSRedirector) newAAAA(name string, ip net.IP) dns.RR {
return &dns.AAAA{
Hdr: dns.RR_Header{
Name: d.withOrigin(name),
Rrtype: dns.TypeAAAA,
Class: dns.ClassINET,
Ttl: uint32(d.ttl),
},
AAAA: ip,
}
}
// Strip the origin from the query.
func (d *DnsRedirector) getQuestionName(req *dns.Msg) string {
func (d *DNSRedirector) getQuestionName(req *dns.Msg) string {
lx := dns.SplitDomainName(req.Question[0].Name)
ql := lx[0 : len(lx)-d.originNumParts]
return strings.ToLower(strings.Join(ql, "."))
......@@ -151,7 +153,7 @@ func flattenIPs(nodes []*autoradio.NodeStatus) []net.IP {
return ips
}
func (d *DnsRedirector) serveDNS(w dns.ResponseWriter, req *dns.Msg) {
func (d *DNSRedirector) serveDNS(w dns.ResponseWriter, req *dns.Msg) {
m := new(dns.Msg)
// Just NACK ANYs
......@@ -184,7 +186,10 @@ func (d *DnsRedirector) serveDNS(w dns.ResponseWriter, req *dns.Msg) {
// Serve all active nodes on every request. We don't
// really care about errors from GetNodes as long as
// some nodes are returned (i.e. stale data from the
// cache is accepted).
// cache is accepted). Also, we need to filter the
// resulting list for nodes whose IP address protocol
// version matches the request type (IPv4 for A
// requests, IPv6 for AAAA).
var ips []net.IP
nodes, _ := d.client.GetNodes()
if len(nodes) > 0 {
......@@ -197,7 +202,7 @@ func (d *DnsRedirector) serveDNS(w dns.ResponseWriter, req *dns.Msg) {
ips = d.publicIps
}
isV6 := (req.Question[0].Qtype == dns.TypeAAAA)
ips = filterIpByProto(ips, isV6)
ips = filterIPByProto(ips, isV6)
// Shuffle the list in random order, and keep only the
// first N results.
......@@ -209,7 +214,12 @@ func (d *DnsRedirector) serveDNS(w dns.ResponseWriter, req *dns.Msg) {
m.SetReply(req)
m.MsgHdr.Authoritative = true
for _, ip := range ips {
rec := d.recordForIp(query, ip, isV6)
var rec dns.RR
if isV6 {
rec = d.newAAAA(query, ip)
} else {
rec = d.newA(query, ip)
}
m.Answer = append(m.Answer, rec)
dnsTargetStats.IncrVar(ipToMetric(ip))
}
......@@ -233,9 +243,9 @@ func (d *DnsRedirector) serveDNS(w dns.ResponseWriter, req *dns.Msg) {
w.WriteMsg(m)
}
// Run starts the DNS servers on the given address (both tcp and udp).
// Start the DNS servers on the given address (both tcp and udp).
// It creates new goroutines and returns immediately.
func (d *DnsRedirector) Run(addr string) {
func (d *DNSRedirector) Start(addr string) {
dns.HandleFunc(d.origin, func(w dns.ResponseWriter, r *dns.Msg) {
d.serveDNS(w, r)
})
......
// The front-end ('fe') code has the purpose of directing user traffic
// where we want it: that is, on a node that is alive and (possibly)
// not overloaded. We do this at two different levels, DNS and HTTP,
// with two slightly different targets: the former is focused on
// availability, while the latter attempts to evenly distribute
// resource usage.
//
// DNS is used to provide at least one address of an active server:
// the capability to return multiple results, and the high-ttl nature
// of the service mean that we can simply return all the active nodes
// on every request, maximizing the chances that at least one of them
// will be active over a longer period of time.
//
// HTTP requests are istantaneous, and we can't rely on the client
// doing retries, so we must point the user at a single, active node
// on every request. There are two different policies, depending on
// the type of the request:
//
// - SOURCE requests must always reach the current master node. Since
// redirects tend to confuse streaming sources, which might have very
// simple HTTP implementations, we simply proxy the stream to the
// master node.
//
// - listener requests must be routed to an available relay, taking
// utilization into account (be it in terms of bandwidth, cpu usage,
// or more). The fact that the DNS layer returns multiple addresses
// provides already a very rough form of load balancing, but for
// accurate bandwidth planning we can't just rely on clients'
// cooperation. So when a client requests a stream using its public
// URL we need to serve a redirect to the desired node, computed
// according to the load balancing policy. This is currently done by
// serving a M3U file pointing directly at the target node's icecast
// daemon (but this may lock clients to that specific target node on
// failure... client reconnection policies still need some
// investigation).
//
/*
Package fe contains the front-end (directly user facing) code for
autoradio.
The front-end code has the purpose of directing user traffic where we
want it: that is, on a node that is alive and (possibly) not
overloaded. We do this at two different levels, DNS and HTTP, with two
slightly different targets: the former is focused on availability,
while the latter attempts to evenly distribute resource usage.
Request Flow
DNS is used to provide clients with at least one address of an active
server: we can simply return a subset of the active nodes on every
request, maximizing the chances that at least one of them will be
active over a longer period of time.
HTTP requests are istantaneous, and clients usually do not implement a
retry behavior, so we must point the user at a single, active node on
every request. There are two different policies, depending on the type
of the request:
- SOURCE requests must always reach the current master node. Since
redirects tend to confuse streaming sources, which might have very
simple HTTP implementations, we simply proxy the stream to the master
node.
- Listener requests must be routed to an available relay, taking
utilization into account (be it in terms of bandwidth, cpu usage, or
more). The fact that the DNS layer returns multiple addresses provides
already a very rough form of load balancing, but for accurate
bandwidth planning we can't just rely on clients' cooperation. So when
a client requests a stream using its public URL we need to serve a
redirect to the desired node, computed according to the load balancing
policy. This is currently done by serving a M3U file pointing directly
at the target node's icecast daemon (but this may lock clients to that
specific target node on failure... client reconnection policies still
need some investigation).
URLs
Autoradio uses a few different types of URLs when interacting with
clients, with different scopes and purposes. For instance, with a
mount named "/test.ogg", we would have:
The main public URL for the stream:
http://stream.${DOMAIN}/test.ogg.
It's a permanent URL that will always be valid. Clients will use
this URL to listen to the stream. M3U files must contain this URL.
The response from the previous URL will be a redirect to a specific IP:
http://${IP}/_stream/test.ogg
This is just a proxy to the Icecast daemon running on that machine.
The chosen IP is either one chosen by the load balancing algorithm, if
the request is a GET from a client, or the current master in case of a
SOURCE request.
Assumptions
There are some fundamental assumptions in the traffic model presented
here, which are not entirely true in the real world:
1) Clients are able to follow HTTP redirects:
Browsers apparently do (though they try really hard to cache the
redirects, which we do not want), but a number of old streaming radio
clients probably don't. Some old streaming audio libraries have really
bare-bones HTTP client implementations. This problem appears to be
even worse for sources.
2) Clients (or users) will retry if the connection is interrupted:
Autoradio servers will usually drop connections on most
cluster-level events, so it is expected that clients will retry and
reconnect. Most clients don't do anything like that unfortunately,
but perhaps users will (if the stream suddenly stops).
*/
package fe
......@@ -10,15 +10,12 @@ import (
"net"
"net/http"
"net/url"
"path"
"path/filepath"
"sort"
"strconv"
"strings"
"time"
_ "net/http/pprof"
"git.autistici.org/ale/autoradio"
"git.autistici.org/ale/autoradio/Godeps/_workspace/src/github.com/PuerkitoBio/ghost/handlers"
"git.autistici.org/ale/autoradio/instrumentation"
......@@ -38,7 +35,8 @@ var (
)
// ResponseWriter wrapper that logs an entry for every incoming HTTP
// request, as soon as the response headers are sent.
// request, as soon as the response headers are sent. This makes more
// sense for long-standing connections like in our case.
type logResponseWriter struct {
http.ResponseWriter
......@@ -91,8 +89,11 @@ func logHandler(h http.Handler) http.HandlerFunc {
}
}
// HTTP redirector.
type HttpRedirector struct {
// HTTPRedirector makes clients talk to Icecast. This can happen
// either by sending HTTP redirects (if you want Icecast to be
// directly reachable from the outside), or by proxying the
// connections (and use just one open port).
type HTTPRedirector struct {
domain string
staticDir string
lb *autoradioLoadBalancer
......@@ -100,7 +101,8 @@ type HttpRedirector struct {
template *template.Template
}
func NewHttpRedirector(client *autoradio.Client, domain, lbspec, staticDir, templateDir string) (*HttpRedirector, error) {
// NewHTTPRedirector creates a new HTTP redirector.
func NewHTTPRedirector(client *autoradio.Client, domain, lbspec, staticDir, templateDir string) (*HTTPRedirector, error) {
lb, err := parseLoadBalancerSpec(lbspec)
if err != nil {
return nil, err
......@@ -108,7 +110,7 @@ func NewHttpRedirector(client *autoradio.Client, domain, lbspec, staticDir, temp
tmpl := template.Must(
template.ParseGlob(
filepath.Join(templateDir, "*.html")))
return &HttpRedirector{
return &HTTPRedirector{
client: client,
domain: domain,
lb: lb,
......@@ -119,10 +121,10 @@ func NewHttpRedirector(client *autoradio.Client, domain, lbspec, staticDir, temp
// Pick a random IP with a protocol appropriate to the request (based
// on the remote address).
func randomIpForRequest(ips []net.IP, r *http.Request) net.IP {
func randomIPForRequest(ips []net.IP, r *http.Request) net.IP {
remoteAddr := net.ParseIP(r.RemoteAddr)
isV6 := (remoteAddr != nil && (remoteAddr.To4() == nil))
return randomIpByProto(ips, isV6)
return randomIPByProto(ips, isV6)
}
type httpRequestContext struct {
......@@ -135,15 +137,15 @@ func (r *httpRequestContext) RemoteAddr() net.IP {
// Return an active node, chosen according to the current load
// balancing policy.
func (h *HttpRedirector) pickActiveNode(r *http.Request) net.IP {
func (h *HTTPRedirector) pickActiveNode(r *http.Request) net.IP {
result := h.lb.Choose(&httpRequestContext{r})
if result == nil {
return nil
}
return randomIpForRequest(result.IP, r)
return randomIPForRequest(result.IP, r)
}
func (h *HttpRedirector) lbUpdater() {
func (h *HTTPRedirector) lbUpdater() {
for range time.NewTicker(2 * time.Second).C {
nodes, err := h.client.GetNodes()
if err != nil {
......@@ -157,7 +159,7 @@ func icecastAddr(server net.IP) string {
return net.JoinHostPort(server.String(), strconv.Itoa(autoradio.IcecastPort))
}
func streamUrl(server net.IP, mountName string) string {
func streamURL(server net.IP, mountName string) string {
var serverAddr string
if *proxyStreams {
serverAddr = server.String()
......@@ -168,7 +170,7 @@ func streamUrl(server net.IP, mountName string) string {
}
// Request wrapper that passes a Mount along with the HTTP request.
func (h *HttpRedirector) withMount(f func(*autoradio.Mount, http.ResponseWriter, *http.Request)) http.Handler {
func (h *HTTPRedirector) withMount(f func(*autoradio.Mount, http.ResponseWriter, *http.Request)) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
mountPath := strings.TrimSuffix(r.URL.Path, ".m3u")
mount, err := h.client.GetMount(mountPath)
......@@ -182,7 +184,7 @@ func (h *HttpRedirector) withMount(f func(*autoradio.Mount, http.ResponseWriter,
// Serve a M3U response. This simply points back at the stream
// redirect handler.
func (h *HttpRedirector) serveM3U(mount *autoradio.Mount, w http.ResponseWriter, r *http.Request) {
func (h *HTTPRedirector) serveM3U(mount *autoradio.Mount, w http.ResponseWriter, r *http.Request) {
m3u := strings.TrimSuffix(r.URL.String(), ".m3u") + "\n"
w.Header().Set("Content-Length", strconv.Itoa(len(m3u)))
w.Header().Set("Content-Type", "audio/x-mpegurl")
......@@ -190,48 +192,8 @@ func (h *HttpRedirector) serveM3U(mount *autoradio.Mount, w http.ResponseWriter,
io.WriteString(w, m3u)
}
// redirect replies to the request with a redirect to url, adding some
// cache-busting headers. Code is mostly verbatim from net/http.
// Serve a 307 for HTTP/1.1 clients, a 302 otherwise.
func redirect(w http.ResponseWriter, r *http.Request, urlStr string) {
if u, err := url.Parse(urlStr); err == nil {
oldpath := r.URL.Path
if oldpath == "" {
oldpath = "/"
}
if u.Scheme == "" {
if urlStr == "" || urlStr[0] != '/' {
olddir, _ := path.Split(oldpath)
urlStr = olddir + urlStr
}
var query string
if i := strings.Index(urlStr, "?"); i != -1 {
urlStr, query = urlStr[:i], urlStr[i:]
}
trailing := strings.HasSuffix(urlStr, "/")
urlStr = path.Clean(urlStr)
if trailing && !strings.HasSuffix(urlStr, "/") {
urlStr += "/"
}
urlStr += query
}
}
w.Header().Set("Location", urlStr)
w.Header().Set("Cache-Control", "max-age=0,no-cache,no-store")
w.Header().Set("Pragma", "no-cache")
w.Header().Set("Expires", "-1")
code := 302
if r.ProtoMinor == 1 {
code = 307
}
w.WriteHeader(code)
}
// Serve a response for a client connection to a relay.
func (h *HttpRedirector) serveRelay(mount *autoradio.Mount, w http.ResponseWriter, r *http.Request) {
func (h *HTTPRedirector) serveRelay(mount *autoradio.Mount, w http.ResponseWriter, r *http.Request) {
// Find an active node.
relayAddr := h.pickActiveNode(r)
if relayAddr == nil {
......@@ -244,13 +206,25 @@ func (h *HttpRedirector) serveRelay(mount *autoradio.Mount, w http.ResponseWrite
if strings.HasSuffix(r.URL.Path, ".m3u") {
h.serveM3U(mount, w, r)
} else {
targetURL := streamUrl(relayAddr, mount.Name)
redirect(w, r, targetURL)
targetURL := streamURL(relayAddr, mount.Name)
// Firefox apparently caches redirects regardless of
// the status code, so we have to add some quite
// aggressive cache-busting headers. We serve a status
// code of 307 to HTTP/1.1 clients, 302 otherwise.
w.Header().Set("Cache-Control", "max-age=0,no-cache,no-store")
w.Header().Set("Pragma", "no-cache")
w.Header().Set("Expires", "-1")
code := 302
if r.ProtoMinor == 1 {
code = 307
}
http.Redirect(w, r, targetURL, code)
}
}
// Handle SOURCE requests.
func (h *HttpRedirector) serveSource(mount *autoradio.Mount, w http.ResponseWriter, r *http.Request) {
func (h *HTTPRedirector) serveSource(mount *autoradio.Mount, w http.ResponseWriter, r *http.Request) {
if mount.IsRelay() {
log.Printf("source: connection to relay stream %s", mount.Name)
http.Error(w, "Stream is relayed", http.StatusBadRequest)
......@@ -316,7 +290,7 @@ func (l mountStatusList) Less(i, j int) bool {
}
// Serve our cluster status page.
func (h *HttpRedirector) serveStatusPage(w http.ResponseWriter, r *http.Request) {
func (h *HTTPRedirector) serveStatusPage(w http.ResponseWriter, r *http.Request) {
nodes, _ := h.client.GetNodes()
mounts, _ := h.client.ListMounts()
......@@ -384,7 +358,7 @@ func withLocalhost(h http.Handler) http.Handler {
})
}
func (h *HttpRedirector) createHandler() http.Handler {
func (h *HTTPRedirector) createHandler() http.Handler {
// Create our HTTP handler stack.
mux := http.NewServeMux()
......@@ -444,7 +418,7 @@ func (h *HttpRedirector) createHandler() http.Handler {
}
// Run starts the HTTP server on the given addr. Does not return.
func (h *HttpRedirector) Run(addr string) {
func (h *HTTPRedirector) Run(addr string) {
// Start the background goroutine that updates the
// LoadBalancer asynchronously.
go h.lbUpdater()
......@@ -462,5 +436,5 @@ func (h *HttpRedirector) Run(addr string) {
func addDefaultHeaders(w http.ResponseWriter) {
w.Header().Set("Expires", "-1")
w.Header().Set("Cache-Control", "private, max-age=0")
w.Header().Set("Cache-Control", "no-store")
}
......@@ -25,7 +25,7 @@ func createTestTargetServer(t *testing.T) (*httptest.Server, int) {
return srv, port
}
func createTestHttpRedirector(t *testing.T) *HttpRedirector {
func createTestHTTPRedirector(t *testing.T) *HTTPRedirector {
nodes := []*autoradio.NodeStatus{
&autoradio.NodeStatus{
Name: "node1",
......@@ -48,7 +48,7 @@ func createTestHttpRedirector(t *testing.T) *HttpRedirector {
`{"Name": "node1", "IP": ["127.0.0.1"]}`,
86400)
client := autoradio.NewClient(etcd)
h, err := NewHttpRedirector(client, "example.com", "best", "./static", "./templates")
h, err := NewHTTPRedirector(client, "example.com", "best", "./static", "./templates")
if err != nil {
t.Fatal(err)
}
......@@ -59,8 +59,8 @@ func createTestHttpRedirector(t *testing.T) *HttpRedirector {
return h
}
func createTestHttpServer(t *testing.T) (*HttpRedirector, *httptest.Server) {
h := createTestHttpRedirector(t)
func createTestHttpServer(t *testing.T) (*HTTPRedirector, *httptest.Server) {
h := createTestHTTPRedirector(t)
srv := httptest.NewServer(h.createHandler())
return h, srv
}
......@@ -86,7 +86,7 @@ func doHttpRequest(t *testing.T, method, url string, expectedStatus int) string
return string(data)
}
func TestHttpRedirector_StatusPage(t *testing.T) {
func TestHTTPRedirector_StatusPage(t *testing.T) {
_, srv := createTestHttpServer(t)
defer srv.Close()
......@@ -106,7 +106,7 @@ func TestHttpRedirector_StatusPage(t *testing.T) {
}
}
func TestHttpRedirector_Static(t *testing.T) {
func TestHTTPRedirector_Static(t *testing.T) {
_, srv := createTestHttpServer(t)
defer srv.Close()
......@@ -116,7 +116,7 @@ func TestHttpRedirector_Static(t *testing.T) {
}
}
func TestHttpRedirector_LBDebugPage(t *testing.T) {
func TestHTTPRedirector_LBDebugPage(t *testing.T) {
_, srv := createTestHttpServer(t)
defer srv.Close()
......@@ -153,7 +153,7 @@ func createTestHttpContext(t *testing.T) *httpTestContext {
return c
}
func TestHttpRedirector_Source(t *testing.T) {
func TestHTTPRedirector_Source(t *testing.T) {
ctx := createTestHttpContext(t)
defer ctx.Close()
......@@ -165,7 +165,7 @@ func TestHttpRedirector_Source(t *testing.T) {
doHttpRequest(t, "SOURCE", ctx.srv.URL+"/nonexist.ogg", 404)
}
func TestHttpRedirector_Relay(t *testing.T) {
func TestHTTPRedirector_Relay(t *testing.T) {
ctx := createTestHttpContext(t)
defer ctx.Close()
......@@ -175,7 +175,7 @@ func TestHttpRedirector_Relay(t *testing.T) {
}
}
func TestHttpRedirector_IcecastProxy(t *testing.T) {
func TestHTTPRedirector_IcecastProxy(t *testing.T) {
*proxyStreams = true
defer func() {
*proxyStreams = false
......
......@@ -3,7 +3,7 @@
<head>
<title>stream.{{.Domain}}</title>
<link rel="stylesheet"
href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.2/css/bootstrap.min.css">
href="//maxcdn.bootstrapcdn.com/bootstrap/3.3.2/css/bootstrap.min.css">
<link rel="stylesheet" href="/static/style.css">
<link rel="shortcut icon" href="/static/radio52.png">
</head>
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment