X Tutup
Skip to content

List Networks need not pull all the endpoints#30673

Merged
tiborvass merged 1 commit intomoby:masterfrom
mavenugo:nr
Feb 4, 2017
Merged

List Networks need not pull all the endpoints#30673
tiborvass merged 1 commit intomoby:masterfrom
mavenugo:nr

Conversation

@mavenugo
Copy link
Contributor

@mavenugo mavenugo commented Feb 2, 2017

Pulling all the endpoints is a very resource heavy operation especially
for Global-scoped networks with a backing KVStore. Such heavy operations
can be fetched for individual network inspect. These are unneccessary
for a simple network list operation.

Signed-off-by: Madhu Venugopal madhu@docker.com

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

@thaJeztah thaJeztah added this to the 1.13.1 milestone Feb 2, 2017
@thaJeztah
Copy link
Member

Looks like this is a change to the API though 😞

@cpuguy83
Copy link
Member

cpuguy83 commented Feb 2, 2017

I'm going to pull this off merge, because the change indeed needs to be addressed in some way.

@mavenugo
Copy link
Contributor Author

mavenugo commented Feb 2, 2017

@cpuguy83 @thaJeztah i think the original implementation is a bug and we are fixing it the correct way here. I have seen other bugs filed for the exact same issue (I will point them when i find it). Also the inspect APIs are available to get the data. I don't think this change is going to break most of the users.

@vdemeester
Copy link
Member

@mavenugo true, but this might break people and we should follow a depreciation policy as we do usually. We need, at least, to version this change (so using the same API version endpoint give the same result)

@mavenugo
Copy link
Contributor Author

mavenugo commented Feb 3, 2017

@vdemeester @thaJeztah @cpuguy83 @tiborvass Added the version check. PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

not tryin to bikeshed but i find the function name a bit weird. How about buildDetailedNetworkResource since NetworkResource is the normal one.

Pulling all the endpoints is a very resource heavy operation especially
for Global-scoped networks with a backing KVStore. Such heavy operations
can be fetched for individual network inspect. These are unneccessary
for a simple network list operation.

Signed-off-by: Madhu Venugopal <madhu@docker.com>
@mavenugo
Copy link
Contributor Author

mavenugo commented Feb 3, 2017

@tiborvass done changing the function name. ptal.

list = append(list, *nr)
}

list, err = filterNetworks(list, netFilters)
Copy link
Member

Choose a reason for hiding this comment

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

As a follow up, it may be worth considering to add information not needed for filtering until after the networks have been filtered.

Perhaps create utility functions to append information such as containers and peers (e.g. appendEndpointInfo(list), appendPeerInfo(list))

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

left a suggestion for possible improvement (follow up), but LGTM on this one

@tiborvass LGTY?

@tiborvass
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

X Tutup