Skip to content

Deprecate "network" and enable "networks" stats (remote Docker API 1.21) (branch master) #361

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.

Sign up for GitHub

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

Closed
bonigarcia wants to merge 2 commits into from
Closed

Deprecate "network" and enable "networks" stats (remote Docker API 1.21) (branch master) #361

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b19bc7
Deprecate "network" and enable "networks" stats (remote Docker API 1.21)
bonigarcia Nov 10, 2015
6ceb3cf
Improve JavaDoc and include @CheckForNull
bonigarcia Nov 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion src/main/java/com/github/dockerjava/api/model/Statistics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

import java.util.Map;

import javax.annotation.CheckForNull;

import org.apache.commons.lang.builder.ToStringBuilder;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonInclude.Include;
import com.fasterxml.jackson.annotation.JsonProperty;

/**
* Representation of a Docker statistics.
Expand All @@ -19,6 +21,17 @@ public class Statistics {
@JsonProperty("read")
private String read;

/**
* @since Docker Remote API 1.21
*/
@CheckForNull
@JsonProperty("networks")
Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

Plus add @CheckForNull

private Map<String, Object> networksStats;
Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

Add java doc with @since to indicate when it appeared

Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

Object under question, doesn't it have some better structure?

Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

Sounds like over naming. Class itself about stats and naming vars with stats tail under question.
@marcuslinke imho better name it as is to be closer to docker API docs. Hopefully for this variable docker devs placed normal name.

Copy link
Contributor

@marcuslinke marcuslinke Nov 10, 2015

Choose a reason for hiding this comment

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

Variables are normally named like JSON fields in docker-java. But what about getter? getNetwork()/getNetworks() should return an dedicated object instead of a map, right?

Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

I see no specific structure and wrappers for it. Until it just a map iface:stats Map looks good. But i don't like <Object> how it expected for usage?

Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

Or do you want create the same analogue as PortBindings?

Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

Sounds like design question. Up to @marcuslinke

Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

Then @marcuslinke should create 2.x or 2.1 branch

Copy link
Contributor

@marcuslinke marcuslinke Nov 10, 2015

Choose a reason for hiding this comment

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

There are API breaking changes already in master, so this will not work. We should really consider another branching model...

Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

@marcuslinke just create new branch based on previous to cc5b11a (hope it has no bad changes before) then @bonigarcia will PR to this branch and you will do 2.1.3 in parallel.

Copy link
Contributor

@marcuslinke marcuslinke Nov 10, 2015

Choose a reason for hiding this comment

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

OK. Done. Created branch 2.x

Copy link
Contributor Author

@bonigarcia bonigarcia Nov 11, 2015

Choose a reason for hiding this comment

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

PR to branch 2.x in #362


/**
* @deprecated as of Docker Remote API 1.21, replaced by {@link #networksStats}
*/
@Deprecated
@JsonProperty("network")
private Map<String, Object> networkStats;

Expand All @@ -31,6 +44,18 @@ public class Statistics {
@JsonProperty("cpu_stats")
private Map<String, Object> cpuStats;

/**
* @since Docker Remote API 1.21
*/
@CheckForNull
public Map<String, Object> getNetworksStats() {
Copy link
Member

@KostyaSha KostyaSha Nov 10, 2015

Choose a reason for hiding this comment

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

Plus add @CheckForNull

return networksStats;
}

/**
* @deprecated as of Docker Remote API 1.21, replaced by {@link #getNetworksStats()}
*/
@Deprecated
public Map<String, Object> getNetworkStats() {
return networkStats;
}
Expand Down