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

[Bug][API] list paging missing totalpage #15619

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Conversation

Gallardot
Copy link
Member

@Gallardot Gallardot commented Feb 23, 2024

Purpose of the pull request

#15181 deleted some code by mistake

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

Signed-off-by: Gallardot <gallardot@apache.org>
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 38.53%. Comparing base (24ff70d) to head (c494c74).

❗ Current head c494c74 differs from pull request most recent head 2d59ec9. Consider uploading reports for the commit 2d59ec9 to get more accurate results

Files Patch % Lines
...rg/apache/dolphinscheduler/api/utils/PageInfo.java 57.14% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##                dev   #15619   +/-   ##
=========================================
  Coverage     38.53%   38.53%           
- Complexity     4783     4784    +1     
=========================================
  Files          1316     1316           
  Lines         45036    45043    +7     
  Branches       4821     4823    +2     
=========================================
+ Hits          17354    17357    +3     
- Misses        25792    25793    +1     
- Partials       1890     1893    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented Feb 23, 2024

@SbloodyS
Copy link
Member

SbloodyS commented Feb 23, 2024

#15148 is closed. Which deleted some code by mistake? @Gallardot

@SbloodyS
Copy link
Member

Did you mean #15181?

@SbloodyS SbloodyS added bug Something isn't working 3.2.2 labels Feb 23, 2024
@SbloodyS SbloodyS added this to the 3.2.2 milestone Feb 23, 2024
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

LGTM.

@SbloodyS
Copy link
Member

cc @ruanwenjun

@Gallardot
Copy link
Member Author

Did you mean #15181?

Yes, #15181

@SbloodyS SbloodyS merged commit eb2a75b into apache:dev Feb 23, 2024
56 checks passed
@Gallardot Gallardot deleted the fix-pageinfo branch February 23, 2024 07:00
Copy link

@DaZuiZui DaZuiZui left a comment

Choose a reason for hiding this comment

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

This seems to achieve the same function, and is simpler
this.totalPage = Math.ceil(this.total / this.pageSize);

@@ -75,4 +77,15 @@ public static <T> PageInfo<T> of(IPage<T> iPage) {
public static <T> PageInfo<T> of(Integer currentPage, Integer pageSize) {
return new PageInfo<>(currentPage, pageSize);
}

public Integer getTotalPage() {

Choose a reason for hiding this comment

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

This seems to achieve the same function, and is simpler
this.totalPage = Math.ceil(this.total / this.pageSize);

Copy link
Member Author

Choose a reason for hiding this comment

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

@DaZuiZui
The original code is from a previously deleted section #15181. But your suggestions are better, there is no reason not to optimize, you are welcome to create a PR❤️.

Copy link
Member

Choose a reason for hiding this comment

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

@DaZuiZui The original code is from a previously deleted section #15181. But your suggestions are better, there is no reason not to optimize, you are welcome to create a PR❤️.

+1

Choose a reason for hiding this comment

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

I have submitted PR
#15620

@ruanwenjun
Copy link
Member

Thanks for fixing this Bug?

Gallardot added a commit to Gallardot/dolphinscheduler that referenced this pull request Mar 14, 2024
Signed-off-by: Gallardot <gallardot@apache.org>
Gallardot pushed a commit to Gallardot/dolphinscheduler that referenced this pull request Mar 14, 2024
[Bug][API] list paging missing totalpage (apache#15619)

See merge request logan/devops/apache/dolphinscheduler!21
@niyanchun
Copy link

our production environment meet this problem, When 3.2.2 release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Version 3.2.1 [Instance list paging is incorrect, the monitored disk capacity is displayed as 0]
6 participants