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

Eric/goal2 569 multiple srv servers #121

Merged
merged 9 commits into from Jul 19, 2019

Conversation

@egieseke
Copy link
Contributor

egieseke commented Jul 5, 2019

Summary

Support retrieving peer addresses using multiple SRV records

Test Plan

Ran the existing set of unit tests.

egieseke added 3 commits Jun 24, 2019
synch with master
Synch with master (7/2/2019)
@egieseke egieseke requested a review from algobolson Jul 5, 2019
wn.log.Debugf("got no DNS addrs for network %#v", wn.NetworkID)
}
if dnsStatus {
wn.phonebook = multiPhonebook

This comment has been minimized.

Copy link
@Karmastic

Karmastic Jul 5, 2019

Contributor

wn.phonebook needs to be set only once. There are several sources that contribute to this multi-phonebook and DNSBoostrap results are only one.
I would rewrite to store a map of boostrapIDs to ThreadsafePhonebook. Then a refresh would be: add/update dnsPhonebooks, mp.AddPhonebook(addedOrUpdated). We can assume for now we'll never have to REMOVE since this DNSBootstrapArray is based on a fixed string (from config.json). The key is you can't just replace wn.phonebook as that wipes out other entries (like the -p set from the cmdline)

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 8, 2019

Author Contributor

Added map of phonebooks to MultiPhonebook. Replace the map entries when updating DNS Bootstrap entries for SRV records.

}
sizes := make([]int, len(mp.phonebooks))

if len(phonebookList) == 1 {

This comment has been minimized.

Copy link
@Karmastic

Karmastic Jul 8, 2019

Contributor

Change for loop: if len(mp.phonebookMap) == 0 { return phonebook.GetAddresses(n) }

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 8, 2019

Author Contributor

There is no longer a phonebook variable to get addresses from, only the phonebook map which includes a "default" phonebook.

This comment has been minimized.

Copy link
@Karmastic

Karmastic Jul 8, 2019

Contributor

The for loop has a phonebook iterator - that's what I meant.

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

Updated to iterate over the map.

@@ -681,6 +681,23 @@ func loadConfig(reader io.Reader, config *Local) error {
return dec.Decode(config)
}

// DNSBootstrapArray returns an array of one or more DNS Bootstrap identifiers
func (cfg Local) DNSBootstrapArray(networkID protocol.NetworkID) (bootstrapArray []string) {
dnsBootstrapString := cfg.DNSBootstrap(networkID)

This comment has been minimized.

Copy link
@Karmastic

Karmastic Jul 8, 2019

Contributor

Why not just return strings.Split(bootstrapString, ";") ?

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 8, 2019

Author Contributor

It does not handle all cases, for example, if there is a leading or trailing semicolon or whitespace. https://medium.com/@mlowicki/strings-fieldsfunc-vs-strings-split-96c667912f78

This comment has been minimized.

Copy link
@tsachiherman

tsachiherman Jul 8, 2019

Contributor

you can always use strings.Split(bootstrapString, "; "), right ?
it's much more intuitive than the code below.
We don't have to support any user input here. The config file is assumed to be crafted correctly.

This comment has been minimized.

Copy link
@Karmastic

Karmastic Jul 8, 2019

Contributor

and if it's a concern, consumer can trim and ignore empties while iterating (or we could do the same and return a sanitized array). This approach is overly complicated.

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

I started with Split() but updated to use the FieldsFunc() with white space removal so that the test cases would work. I will revert to split and remove some of the test cases from config_test.TestLocal_DNSBootstrapArray().

@@ -163,14 +175,9 @@ func (mp *MultiPhonebook) GetAddresses(n int) []string {
return out
}

// AddPhonebook adds a Phonebook if it is new
func (mp *MultiPhonebook) AddPhonebook(p Phonebook) {
// AddOrUpdatePhonebook adds or updates Phonebook in Pnonebook map

This comment has been minimized.

Copy link
@tsachiherman

tsachiherman Jul 8, 2019

Contributor

nit: typo Pnonebook -> PhonebookMap

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

fixed

// AddPhonebook adds a Phonebook if it is new
func (mp *MultiPhonebook) AddPhonebook(p Phonebook) {
// AddOrUpdatePhonebook adds or updates Phonebook in Pnonebook map
func (mp *MultiPhonebook) AddOrUpdatePhonebook(name string, p Phonebook) {

This comment has been minimized.

Copy link
@tsachiherman

tsachiherman Jul 8, 2019

Contributor

Could you please give the variable "name" into some more descriptive name ? maybe bootstrapNetworkName ?

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

done

}
multiPhonebook.AddOrUpdatePhonebook(dnsBootstrap, dnsPhonebook)
} else {
wn.log.Debugf("got no DNS addrs for network %#v", wn.NetworkID)

This comment has been minimized.

Copy link
@tsachiherman

tsachiherman Jul 8, 2019

Contributor
  1. Change into "Infof"
  2. don't use %#v everywhere. in this case, I think that %s would do.

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

updated

// AddPhonebook adds a Phonebook if it is new
func (mp *MultiPhonebook) AddPhonebook(p Phonebook) {
// AddOrUpdatePhonebook adds or updates Phonebook in Pnonebook map
func (mp *MultiPhonebook) AddOrUpdatePhonebook(name string, p Phonebook) {

This comment has been minimized.

Copy link
@algobolson

algobolson Jul 8, 2019

Collaborator

Why wasn't AddPhonebook() enough? It filtered out duplicates and updates to the underlying data were visible because Phonebook is an interface reference type.

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

the underlying map of phonebooks requires key-value pairs.

This comment has been minimized.

Copy link
@algobolson

algobolson Jul 9, 2019

Collaborator

but it doesn't need to be a map. we never need to 'update(phonebookName, phonebookReference)' because it's already a reference and the value is never stale. it already de-duplicated phonebooks added.

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

That is how I started, but it was suggested to manage the DNS bootstrap entries as a map utilizing the MutliPhonebook.

if ok {
mp.AddPhonebook(&wn.dnsPhonebook)
dnsBootstrapArray := wn.config.DNSBootstrapArray(wn.NetworkID)
multiPhonebook := MakeMultiPhonebook()

This comment has been minimized.

Copy link
@algobolson

algobolson Jul 8, 2019

Collaborator

it looks like nothing consumes any data gathered into multiPhonebook

This comment has been minimized.

Copy link
@Karmastic

Karmastic Jul 8, 2019

Contributor

Yeah - i was just noticing that too. I think this should be mp.phonebookMap -- we shouldn't have a local multiPhonebook instance.

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

good catch, fixed.

addrs := make([][]string, len(mp.phonebooks))
for pi, p := range mp.phonebooks {
addrs := make([][]string, len(phonebookList))
for pi, p := range phonebookList {

This comment has been minimized.

Copy link
@Karmastic

Karmastic Jul 8, 2019

Contributor

We don't need a phonebookList array at all - just iterate over phonebookMap, and put the len(map) == 0 check in this one loop.

This comment has been minimized.

Copy link
@egieseke

egieseke Jul 9, 2019

Author Contributor

removed phonebookList array.

Copy link
Contributor

Karmastic left a comment

Looks good to me. I didn't test it so I'm trusting that you verified you can run and connect to devnet or another network.

@@ -175,6 +182,12 @@ func (mp *MultiPhonebook) GetAddresses(n int) []string {
return out
}

func (mp *MultiPhonebook) addAddressArrayToAdressSet(addrMap *map[string]bool, addrArray *[]string) {

This comment has been minimized.

Copy link
@algobolson

algobolson Jul 11, 2019

Collaborator

doesn't actually need to be a member of MultiPhonebook since it never uses the mp reference. generically it might be stringMapSetUpdate(...)

@winder winder merged commit 6193403 into algorand:master Jul 19, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
license/cla Contributor License Agreement is signed.
Details
tsachiherman added a commit to tsachiherman/go-algorand that referenced this pull request Jul 19, 2019
Eric/goal2 569 multiple srv servers (algorand#121)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.