Adding pagination utility #527

karthikkondapally · 2019-03-13T21:55:04Z

pagination tool #523
@brendandburns @yue9944882

brendandburns · 2019-03-14T18:40:56Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

+  private Integer limit;
+  private String _continue;
+
+  public PagerParams() {}


brendandburns · 2019-03-14T18:42:12Z

util/src/test/java/io/kubernetes/client/pager/PagerTest.java

+    client = new ClientBuilder().setBasePath("http://localhost:" + PORT).build();
+  }
+
+  @Test


Let's add/expand the tests so that they actually cover the pagination.

+1. add let's add test when the list receives a 400 due to invalid limit or continue, maybe expecting a checked exception here?

added 2 more tests for failure cases.

brendandburns · 2019-03-14T18:42:46Z

util/src/main/java/io/kubernetes/client/pager/Pager.java

+  /**
+   * Pagination in kubernetes list call depends on continue and limit variable
+   *
+   * @param listFunc


Either document the parameters, or delete these. (and below)

brendandburns · 2019-03-14T18:43:03Z

util/src/main/java/io/kubernetes/client/pager/Pager.java

@@ -0,0 +1,107 @@
+package io.kubernetes.client.pager;


Add copyright boilerplate here.

brendandburns · 2019-03-14T18:43:32Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

@@ -0,0 +1,30 @@
+package io.kubernetes.client.util;


Add boilerplate here.

brendandburns · 2019-03-14T18:44:19Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

@@ -0,0 +1,30 @@
+package io.kubernetes.client.util;


It's weird that this is in a different package than Pager. move to pager? or util.pager

moved to pager package.

brendandburns · 2019-03-14T18:44:49Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

+
+public class PagerParams {
+
+  private Integer limit;


nit, I prefer int to Integer but I don't feel strongly.

i'm fine w/ either. a zero int and a null Integer actually works the same.

https://github.com/kubernetes/kubernetes/blob/d5a3db003916b1d33b503ccd2e4897e094d8af77/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go#L494-L497

brendandburns · 2019-03-14T18:45:01Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

+public class PagerParams {
+
+  private Integer limit;
+  private String _continue;


nit: remove underscore.

isn't continue a reserved key word?

good point, but perhaps continueToken? underscore is pretty unusual in Java variable names...

changed to continueToken.

brendandburns · 2019-03-14T18:45:14Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

+    return _continue;
+  }
+
+  public void setContinue(String _continue) {


remove underscore.

continueToken as above?

changed to continueToken.

brendandburns · 2019-03-14T18:45:43Z

This is great! Some small-ish comments.

yue9944882 · 2019-03-15T03:17:48Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

+public class PagerParams {
+
+  private Integer limit;
+  private String _continue;


isn't continue a reserved key word?

yue9944882 · 2019-03-15T03:19:36Z

util/src/main/java/io/kubernetes/client/pager/Pager.java

+import java.lang.reflect.Type;
+import java.util.function.Function;
+
+public class Pager {


if we don't want to make the pager shared by multiple resource types, better make it type parameterized.

Suggested change

public class Pager {

public class Pager<ApiType, ApiListType> {

yue9944882 · 2019-03-15T03:20:15Z

util/src/main/java/io/kubernetes/client/pager/Pager.java

+   *
+   * @return Object
+   */
+  public <T> T next() {


nit: s/<ApiType>/<T>/g

yue9944882 · 2019-03-15T03:21:55Z

util/src/test/java/io/kubernetes/client/pager/PagerTest.java

+    client = new ClientBuilder().setBasePath("http://localhost:" + PORT).build();
+  }
+
+  @Test


+1. add let's add test when the list receives a 400 due to invalid limit or continue, maybe expecting a checked exception here?

yue9944882 · 2019-03-15T03:22:35Z

examples/src/main/java/io/kubernetes/client/examples/PagerExample.java

+  public static void main(String[] args) throws IOException {
+
+    ApiClient client = Config.defaultClient();
+    client.getHttpClient().setReadTimeout(60, TimeUnit.SECONDS);


(might be an unrelated question) what's the point of this?

This just prevents timeouts for large bodies/slow connections. I think the default is 10 seconds?

It could be that this is cargo-cult copied from other examples :)

yue9944882 · 2019-03-15T03:32:02Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

+
+public class PagerParams {
+
+  private Integer limit;


i'm fine w/ either. a zero int and a null Integer actually works the same.

https://github.com/kubernetes/kubernetes/blob/d5a3db003916b1d33b503ccd2e4897e094d8af77/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go#L494-L497

yue9944882 · 2019-03-15T03:34:21Z

util/src/main/java/io/kubernetes/client/util/PagerParams.java

+    return limit;
+  }
+
+  public void setLimit(Integer limit) {


how about making the limit an immutable value here? (which is, settable by constructor but didn't have setter/getter). otherwise, what's the use-case of changing page size dynamically?

brendandburns · 2019-03-18T15:30:59Z

This is good enough for now, I'd still like to see a little more coverage in the unit tests, but we can defer to an additional PR.

/lgtm
/approve

k8s-ci-robot · 2019-03-18T15:31:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, kondapally1989

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [brendandburns]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

brendandburns · 2019-03-18T15:31:57Z

@yue9944882 @adohe @kondapally1989

Let's move this into the extended module (separate PR after this merges is fine)

karthikkondapally added 5 commits March 10, 2019 23:10

Pagination initial commit #523

19a749c

move add to set query

50f7bdf

fix issues

46233de

add PagerParams ResponseUtils

2d49703

add listFn, refactor, add unit tests

97e4a57

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 13, 2019

k8s-ci-robot requested review from brendandburns and mbohlool March 13, 2019 21:55

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2019

brendandburns reviewed Mar 14, 2019

View reviewed changes

yue9944882 reviewed Mar 15, 2019

View reviewed changes

address code review comments

2749d5b

k8s-ci-robot assigned brendandburns Mar 18, 2019

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 18, 2019

k8s-ci-robot merged commit d600430 into kubernetes-client:master Mar 18, 2019

yue9944882 mentioned this pull request Mar 21, 2019

[Umbrella Issue] Java SDK utility tooling improvement plan #523

Closed

10 tasks

	public class Pager {
	public class Pager<ApiType, ApiListType> {

Adding pagination utility #527

Adding pagination utility #527

Uh oh!

Conversation

karthikkondapally commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Mar 18, 2019

Uh oh!

k8s-ci-robot commented Mar 18, 2019

Uh oh!

brendandburns commented Mar 18, 2019

Uh oh!

Uh oh!