Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generate additional SRV records if port name is specified #386

Merged
merged 2 commits into from
Mar 3, 2016

Conversation

jdef
Copy link
Contributor

@jdef jdef commented Jan 15, 2016

related to #61

summary: if s task provides a discoveryInfo w/ named ports then mesos-dns will generate additional SRV records for those ports: _{di-port-name}._{task-name}._{protocol}.{framework}.{domain}.

@jdef jdef added the WIP label Jan 15, 2016
@jdef jdef closed this Jan 15, 2016
@jdef jdef reopened this Jan 15, 2016
@jdef jdef added WIP and removed WIP labels Jan 15, 2016
@sargun
Copy link
Contributor

sargun commented Mar 1, 2016

Technically RFC non-compliant, but RFCs be damned, productivity ftw.

@jdef
Copy link
Contributor Author

jdef commented Mar 2, 2016

considering _{portname}._sub._{taskname}._{protocol} since it's just as expressive but probably more compat with existing tooling that understands dns-sd (not that we're super concerned about implementing dns-sd)

@jdef
Copy link
Contributor Author

jdef commented Mar 2, 2016

@sargun
Copy link
Contributor

sargun commented Mar 2, 2016

I prefer the current patch. It's something we're already doing, and I think preserving the SRV-style records follows the principal of least surprise. If we choose to support dns-sd, I'd rather create both records.

@jdef jdef added PTAL and removed WIP labels Mar 3, 2016
@jdef
Copy link
Contributor Author

jdef commented Mar 3, 2016

understood. i still modified the current patch to simply prepend a
_portname if there is one. so it's possible to get
_portname._taskname._protocol .. but still think that's ok

On Wed, Mar 2, 2016 at 4:37 PM, Sargun Dhillon notifications@github.com
wrote:

I prefer the current patch. It's something we're already doing, and I
think preserving the SRV-style records follows the principal of least
surprise. If we choose to support dns-sd, I'd rather create both records.


Reply to this email directly or view it on GitHub
#386 (comment).

records[i] += "._" + protocol + "." + framework
}
} else {
records = append(records, records...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to reuse records, and do this in place? Does Golang actually care that much or optimize that much do this in place? Can we just create another slice, and populate that? I think that would make this code much easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be more efficient if the underlying capacity of the slice is
large enough to hold twice the record count. in this particular instance it
depends how golang is allocating the initial slice for varargs parameter.
experiments on playground suggest that it allocates a slice w/ a capacity
equivalent to the # of variadic params.

even so, I'm loath to changing this line: it's concise and idiomatic.

On Thu, Mar 3, 2016 at 1:00 PM, Sargun Dhillon notifications@github.com
wrote:

In records/chains.go
#386 (comment):

+const (

  • protocolNone = "" // for readability
  • domainNone = "" // for readability
    +)

+// withProtocol appends ._{protocol}.{framework} to records. if protocol is "" then
+// the protocols "tcp" and "udp" are assumed.
+func withProtocol(protocol, framework string, spec labels.Func, gen chain) chain {

  • return func(records ...string) {
  •   protocol = spec(protocol)
    
  •   if protocol != protocolNone {
    
  •       for i := range records {
    
  •           records[i] += "._" + protocol + "." + framework
    
  •       }
    
  •   } else {
    
  •       records = append(records, records...)
    

Is there any reason to reuse records, and do this in place? Does Golang
actually care that much or optimize that much do this in place? Can we just
create another slice, and populate that? I think that would make this code
much easier to read.


Reply to this email directly or view it on GitHub
https://github.com/mesosphere/mesos-dns/pull/386/files#r54919995.

@sargun
Copy link
Contributor

sargun commented Mar 3, 2016

So, I like FP, but the lambda passing doesn't seem as idiomatic (in go) that I hoped for. I found the code really hard to read, but I don't think that's a matter of code-quality, I think that it's just a matter of statements like:


// withNamedPort prepends a `_{discoveryInfo port name}.` to records
func withNamedPort(portName string, spec labels.Func, gen chain) chain {
    portName = spec(portName)
    if portName == "" {
        return gen
    }
    return func(records ...string) {
        // generate without port-name prefix
        gen(records...)

        // generate with port-name prefix
        for i := range records {
            records[i] = "_" + portName + "." + records[i]
        }
        gen(records...)
    }
}

-- and since it's passing around lambdas that all have the same type signature / name, and doing all of the manipulations of data types in place, it makes it difficult to follow. I'm not saying we should change it, I'm just wondering if we have a better way to structure this flow.

I'm curious as to whether using goroutines as generators, and channels to wire them all up might be a better option. I know that's less efficient, but as a general pattern I like it, and since you're only processing one item at a time, it should be memory efficient, and not require interesting scheduling.

@sargun
Copy link
Contributor

sargun commented Mar 3, 2016

lgtm.

@jdef
Copy link
Contributor Author

jdef commented Mar 3, 2016

thanks @sargun. bombs away

jdef added a commit that referenced this pull request Mar 3, 2016
generate additional SRV records if port name is specified
@jdef jdef merged commit 71d13a8 into master Mar 3, 2016
@jdef jdef deleted the jdef_port_name_srv branch March 3, 2016 19:32
@jdef jdef removed the PTAL label Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants