Support pagination in sessions listing API #1173

tosametal · 2019-07-11T10:06:22Z

Support pagination on WorkflowsPage, ProjectsPage and ProjectPage(#1106 is related)

I made the docker image, so you can check the ui with the following command.

docker pull tosametal/digdag-pagination:0.0.1
docker run -d -p 65432:65432 tosametal/digdag-pagination:0.0.1

→ access to http://localhost:65432

tosametal · 2019-07-12T05:39:52Z

I also created an image to show the loading when fetching sessions.
tosametal/digdag-pagination:loading3

Please check this too and compare which looks better.

hiroyuki-sato · 2019-07-12T09:18:54Z

Hello, @tosametal

Thank you for creating this PR.

Does the difference between tosametal/digdag-pagination:loading3 and
tosametal/digdag-pagination:0.0.1 is that whether show loading image or not?

If so, I like Loading version.

I confirmed the following steps.

no loading version

docker pull tosametal/digdag-pagination:0.0.1
docker run -d -p 65432:65432 tosametal/digdag-pagination:0.0.1

digdag init -t sh fuga
cd fuga
digdag push fuga

# macOS
for i in `seq 0 120` ; do
  d=`date -v+${i}d -j -f "%Y-%m-%d" "2019-01-01" +"%Y-%m-%d"` 
  digdag start fuga fuga --session $d  
done

Loading version

docker pull tosametal/digdag-pagination:loading3
docker run -d -p 65432:65432 tosametal/digdag-pagination:loading3

digdag init -t sh fuga
cd fuga
digdag push fuga

# macOS
for i in `seq 0 120` ; do
  d=`date -v+${i}d -j -f "%Y-%m-%d" "2019-01-01" +"%Y-%m-%d"` 
  digdag start fuga fuga --session $d  
done

tosametal · 2019-07-12T13:19:56Z

@hiroyuki-sato Thank you for confirmation.

Does the difference between tosametal/digdag-pagination:loading3 and
tosametal/digdag-pagination:0.0.1 is that whether show loading image or not?

Yes, that's the only difference.

If so, I like Loading version.

OK. I will add a commit that shows the loading image to this PR.

When the number of pages is more than 10, the button display changes. Please try it.

hiroyuki-sato · 2019-07-12T13:27:05Z

Hello, @tosametal

This is just my(not committer) opinion.

What do you think? @yoyama, @muga, and @szyn?

hiroyuki-sato · 2019-07-16T08:09:39Z

Hello, @tosametal

When the number of pages is more than 10, the button display changes. Please try it.

It seems to work well.

docker pull tosametal/digdag-pagination:loading3
docker run -d -p 65432:65432 tosametal/digdag-pagination:loading3

digdag init -t sh fuga
cd fuga
digdag push fuga

# macOS
for i in `seq 0 1100` ; do
  d=`date -v+${i}d -j -f "%Y-%m-%d" "2016-01-01" +"%Y-%m-%d"` 
  digdag start fuga fuga --session $d  
done

yoyama

Thanks for your PR. I took a brief look.
Please check the comments.
Current implementation will cause problem in large use case.
In addition please take care of API compatibility, method signature of common classes as much as possible.

digdag-core/src/main/java/io/digdag/core/database/DatabaseSessionStoreManager.java

digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java

yoyama · 2019-07-16T10:18:45Z

In addition I propose split this ticket to UI part and API part for our review.

tosametal · 2019-07-16T14:16:09Z

@hiroyuki-sato Thank you for confirmation. I confirmed it as follows.

# sample1.dig
timezone: 'Asia/Tokyo'
schedule:
  minutes_interval>: 1
+task:
  sh>: echo hoge

for i in `seq 2 100`
do
  cp sample1.dig sample$i.dig
done

digdag push project -e http://localhost:65432

and wait 10 minutes...

tosametal · 2019-07-16T14:16:34Z

@yoyama Thank you for your review. I will check the comments and fix them.

In addition I propose split this ticket to UI part and API part for our review.

Does this mean I should close this PR and create two new PRs?

yoyama · 2019-07-17T03:04:16Z

@tosametal You don't need to close this PR.
I suppose that remove UI codes from this PR and then create follow up PR which include UI code.
How about this way?

tosametal · 2019-07-18T08:56:01Z

@yoyama I agree with your suggestion.I will do that.

This reverts commit 432b8bc.

This reverts commit 7b385f4.

This reverts commit 1520d93.

yoyama · 2019-07-31T07:21:31Z

@tosametal If you finished update, please ping me.

tosametal · 2019-08-26T07:31:51Z

@yoyama I checked your comments, and fix the code.Please check the commits below.

ff63080
2fecfdf

yoyama · 2019-09-03T03:56:01Z

@tosametal Thanks for you fix. I will check it later.

yoyama · 2019-09-03T05:35:15Z

I updated the title. I misunderstood this PR.
This PR try to add page_number parameter to '/api/sessions/{id}' and '/api/projects/{id}/sessions'.
Both are list of sessions.

yoyama · 2019-09-03T05:54:21Z

digdag-server/src/main/java/io/digdag/server/rs/SessionResource.java


-            return RestModels.sessionCollection(rs, sessions);
+            Integer page = Optional.fromNullable(pageNumber).or(1);


The variable name page is uncertain.
It may be better to define QueryParamValidator.validatePageNumber and change the name to validPageNumber.

yoyama · 2019-09-03T05:56:53Z

digdag-server/src/main/java/io/digdag/server/rs/SessionResource.java

@@ -64,17 +65,20 @@ public SessionResource(
    @Path("/api/sessions")
    public RestSessionCollection getSessions(
            @QueryParam("last_id") Long lastId,
-            @QueryParam("page_size") Integer pageSize)
+            @QueryParam("page_size") Integer pageSize,
+            @QueryParam("page_number") Integer pageNumber)


I noticed if page_number <=0, API become error in SQL execution.
I guess it is better to validate before execute query.
But page_size is also same behavior.
It is not mandatory and another PR which fix both page_size and page_number may be better.

OK, I will fix the problem in another PR.

yoyama · 2019-09-03T05:59:33Z

digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java

@@ -425,15 +427,18 @@ public RestSessionCollection getSessions(
            SessionStore ss = ssm.getSessionStore(getSiteId());

            StoredProject proj = ensureNotDeletedProject(ps.getProjectById(projectId));
+            Integer page = Optional.fromNullable(pageNumber).or(1);


Same as SessionResource. Please change var name.

yoyama · 2019-09-03T06:38:42Z

digdag-core/src/main/java/io/digdag/core/database/DatabaseSessionStoreManager.java

+        }
+
+        @Override
+        public Integer getTotalSessionsCount(Optional<Long> lastId)


getSessionsCount may be better name.

yoyama · 2019-09-03T06:40:02Z

digdag-core/src/main/java/io/digdag/core/database/DatabaseSessionStoreManager.java

+        }
+
+        @Override
+        public Integer getTotalProjectSessionsCount(Optional<Long> lastId, int projectId)


getSessionsCountOfProject may be better to fit getSessionsOfProject.

tosametal · 2019-09-04T05:39:55Z

@yoyama I added validatePageNumber to QueryParamValidator and fixed variable and method name.Please check it.

yoyama · 2019-09-30T09:27:58Z

@tosametal Sorry for my late response.
I just started to review your latest PR.
Please wait for a moment.
Your PR is large, so I need to check carefully.

yoyama · 2019-10-02T05:00:16Z

digdag-core/src/main/java/io/digdag/core/database/DatabaseSessionStoreManager.java

+        }
+
+        @Override
+        public List<StoredSessionWithLastAttempt> getSessions(int pageSize, Optional<Long> lastId, int pageNumber)


I guess switch the order of pageNumber and lastId may be understandable.
Other method also same.

yoyama · 2019-10-02T05:03:29Z

digdag-core/src/main/java/io/digdag/core/database/DatabaseSessionStoreManager.java

+                " join session_attempts sa on sa.id = s.last_attempt_id" +
+                " where s.project_id = :projectId" +
+                " and s.id < :lastId")
+        Integer getSessionsCountOfProject(@Bind("lastId") long lastId, @Bind("projectId") int projectId);


It looks like this query is different from getSessionsOfProject. Not have site_id condition.
Could you fix it and add test code for *Count() method.
In addition the test data should include different site_id and check.

yoyama

@tosametal Sorry for my late response. I made some comments to your code. please check and fix them.

tosametal · 2019-12-25T09:15:16Z

@yoyama Sorry for my too late response. I fixed the below.Please check them.

switch the order of pageNumber and lastId
add site_id condition and add test

yoyama · 2020-12-11T02:48:19Z

@tosametal I would like to close this ticket once because it is too old and code is conflicted and we are working on v0.10.0 release.
In addition if we add these functionality, it is better to support not only sessions but also attempts and other for the integrity of API design.
If you want to continue, feel free to reopen after v0.10.0 released.

tosametal added 4 commits July 11, 2019 18:45

Fix api

ee4d784

Add pagination

7b385f4

Fix test

1520d93

Fix ReferenceError

432b8bc

yoyama self-requested a review July 12, 2019 04:50

yoyama requested changes Jul 16, 2019

View reviewed changes

digdag-core/src/main/java/io/digdag/core/database/DatabaseSessionStoreManager.java Outdated Show resolved Hide resolved

digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java Outdated Show resolved Hide resolved

yoyama reviewed Jul 16, 2019

View reviewed changes

digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java Outdated Show resolved Hide resolved

tosametal added 6 commits July 18, 2019 18:15

Revert "Fix ReferenceError"

5ce36f4

This reverts commit 432b8bc.

Revert "Add pagination"

611f54f

This reverts commit 7b385f4.

Restored deleted page size parameters for compatibility

394596a

Revert "Fix test"

b712951

This reverts commit 1520d93.

Add totalPageCount to RestSessionCollection

3f4fe5c

Use RestSessionCollection as return class for compatibility

22431da

tosametal mentioned this pull request Jul 18, 2019

Support Pagination UI part #1182

Closed

tosametal and others added 5 commits July 23, 2019 11:51

Fix to get sessions of specified page only

f94934a

Fix to get project sessions of specified page only

f96a154

Fix query and indent

c51f3ba

Fix indent

3bd36c9

Remove offset from getAttempts

16d9f78

yoyama changed the title ~~Support pagination in projects and sessions listing API~~ Support pagination in sessions listing API Sep 3, 2019

yoyama reviewed Sep 3, 2019

View reviewed changes

tosametal added 3 commits September 3, 2019 15:51

Fix variable name

559b979

Fix method name

8a93b7a

Validate page number with QueryParamValidator

d19eff4

yoyama reviewed Oct 2, 2019

View reviewed changes

yoyama requested changes Oct 2, 2019

View reviewed changes

tosametal added 2 commits October 4, 2019 11:33

Switch the order of pageNumber and lastId

9d65b0b

Add siteId condition to getSessionsCountOfProject query

09061d8

yoyama modified the milestones: v0.9.40, v0.9.41 Dec 10, 2019

Add test

bc0174c

yoyama modified the milestones: v0.9.41, v0.9.42 Jan 21, 2020

yoyama modified the milestones: v0.9.42, v0.9.43 Jun 29, 2020

yoyama closed this Dec 11, 2020

ehaupt mentioned this pull request Oct 28, 2021

BUG: /api/projects does not support pagination #1672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pagination in sessions listing API #1173

Support pagination in sessions listing API #1173

tosametal commented Jul 11, 2019 •

edited

Loading

tosametal commented Jul 12, 2019 •

edited

Loading

hiroyuki-sato commented Jul 12, 2019

tosametal commented Jul 12, 2019

hiroyuki-sato commented Jul 12, 2019

hiroyuki-sato commented Jul 16, 2019

yoyama left a comment •

edited

Loading

yoyama commented Jul 16, 2019

tosametal commented Jul 16, 2019

tosametal commented Jul 16, 2019

yoyama commented Jul 17, 2019

tosametal commented Jul 18, 2019

yoyama commented Jul 31, 2019

tosametal commented Aug 26, 2019

yoyama commented Sep 3, 2019

yoyama commented Sep 3, 2019

yoyama Sep 3, 2019

yoyama Sep 3, 2019

tosametal Sep 4, 2019 •

edited

Loading

yoyama Sep 3, 2019 •

edited

Loading

yoyama Sep 3, 2019

yoyama Sep 3, 2019

tosametal commented Sep 4, 2019

yoyama commented Sep 30, 2019

yoyama Oct 2, 2019

yoyama Oct 2, 2019

yoyama left a comment

tosametal commented Dec 25, 2019

yoyama commented Dec 11, 2020


		return RestModels.sessionCollection(rs, sessions);
		Integer page = Optional.fromNullable(pageNumber).or(1);

Support pagination in sessions listing API #1173

Support pagination in sessions listing API #1173

Conversation

tosametal commented Jul 11, 2019 • edited Loading

tosametal commented Jul 12, 2019 • edited Loading

hiroyuki-sato commented Jul 12, 2019

no loading version

Loading version

tosametal commented Jul 12, 2019

hiroyuki-sato commented Jul 12, 2019

hiroyuki-sato commented Jul 16, 2019

yoyama left a comment • edited Loading

Choose a reason for hiding this comment

yoyama commented Jul 16, 2019

tosametal commented Jul 16, 2019

tosametal commented Jul 16, 2019

yoyama commented Jul 17, 2019

tosametal commented Jul 18, 2019

yoyama commented Jul 31, 2019

tosametal commented Aug 26, 2019

yoyama commented Sep 3, 2019

yoyama commented Sep 3, 2019

yoyama Sep 3, 2019

Choose a reason for hiding this comment

yoyama Sep 3, 2019

Choose a reason for hiding this comment

tosametal Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

yoyama Sep 3, 2019 • edited Loading

Choose a reason for hiding this comment

yoyama Sep 3, 2019

Choose a reason for hiding this comment

yoyama Sep 3, 2019

Choose a reason for hiding this comment

tosametal commented Sep 4, 2019

yoyama commented Sep 30, 2019

yoyama Oct 2, 2019

Choose a reason for hiding this comment

yoyama Oct 2, 2019

Choose a reason for hiding this comment

yoyama left a comment

Choose a reason for hiding this comment

tosametal commented Dec 25, 2019

yoyama commented Dec 11, 2020

tosametal commented Jul 11, 2019 •

edited

Loading

tosametal commented Jul 12, 2019 •

edited

Loading

yoyama left a comment •

edited

Loading

tosametal Sep 4, 2019 •

edited

Loading

yoyama Sep 3, 2019 •

edited

Loading