You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "swamirishi (via GitHub)" <gi...@apache.org> on 2025/03/27 08:15:40 UTC
[PR] HDDS-12708. Fix Unhealthy Containers API for pagination [ozone]
swamirishi opened a new pull request, #8174:
URL: https://github.com/apache/ozone/pull/8174
## What changes were proposed in this pull request?
Fix Unhealthy Container API to support prev pagination by adding query params prevStartKey and prevLastKey.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12708
## How was this patch tested?
Existing unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12708. Fix Unhealthy Containers API for pagination [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8174:
URL: https://github.com/apache/ozone/pull/8174#issuecomment-2968580717
Thank you very much for the patch. I am closing this PR __temporarily__ as there was no activity recently and it is waiting for response from its author.
It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.
It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.
If you need ANY help to finish this PR, please [contact the community](https://github.com/apache/hadoop-ozone#contact) on the mailing list or the slack channel."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12708. Fix Unhealthy Containers API for pagination [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #8174:
URL: https://github.com/apache/ozone/pull/8174#discussion_r2016357659
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/UnhealthyContainersResponse.java:
##########
@@ -49,6 +49,21 @@ public class UnhealthyContainersResponse {
@JsonProperty("misReplicatedCount")
private long misReplicatedCount = 0;
+ /**
+ * .
+ */
+ @JsonProperty("firstKey")
+ private long firstKey = 0;
+
+
+ /**
+ * Total count of mis-replicated containers.
Review Comment:
Is this comment correct ?
##########
hadoop-ozone/recon-codegen/src/main/java/org/apache/ozone/recon/schema/ContainerSchemaDefinition.java:
##########
@@ -92,9 +91,12 @@ private void createUnhealthyContainersTable() {
.constraint(DSL.constraint("pk_container_id")
.primaryKey(CONTAINER_ID, CONTAINER_STATE))
.constraint(DSL.constraint(UNHEALTHY_CONTAINERS_TABLE_NAME + "ck1")
- .check(field(name("container_state"))
+ .check(field(name(CONTAINER_STATE))
.in(UnHealthyContainerStates.values())))
.execute();
+ dslContext.createIndex("idx_container_state")
Review Comment:
So, we need to consider upgrade scenario. This code will not execute if user has already running Recon in a cluster. You may need to write a class similar to `InitialConstraintUpgradeAction`
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -67,32 +72,43 @@ public ContainerHealthSchemaManager(
* matching the given state will be returned.
* @param state Return only containers in this state, or all containers if
* null
- * @param offset The starting record to return in the result set. The first
- * record is at zero.
+ * @param minContainerId minimum containerId for filter
+ * @param maxContainerId maximum containerId for filter
* @param limit The total records to return
* @return List of unhealthy containers.
*/
public List<UnhealthyContainers> getUnhealthyContainers(
- UnHealthyContainerStates state, int offset, int limit) {
+ UnHealthyContainerStates state, Long minContainerId, Optional<Long> maxContainerId, int limit) {
DSLContext dslContext = containerSchemaDefinition.getDSLContext();
SelectQuery<Record> query = dslContext.selectQuery();
query.addFrom(UNHEALTHY_CONTAINERS);
+ Condition containerCondition;
+ OrderField[] orderField;
+ if (maxContainerId.isPresent() && maxContainerId.get() > 0) {
Review Comment:
if both `minContainerId` and `maxContainerId` are passed in API by UI or client application for getting 2nd page result, how this pagination will work ?because `minContainerId` is ignored.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/UnhealthyContainersResponse.java:
##########
@@ -49,6 +49,21 @@ public class UnhealthyContainersResponse {
@JsonProperty("misReplicatedCount")
private long misReplicatedCount = 0;
+ /**
+ * .
+ */
+ @JsonProperty("firstKey")
+ private long firstKey = 0;
Review Comment:
We can rename this to `startContainerId` of page.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -67,32 +72,43 @@ public ContainerHealthSchemaManager(
* matching the given state will be returned.
* @param state Return only containers in this state, or all containers if
* null
- * @param offset The starting record to return in the result set. The first
- * record is at zero.
+ * @param minContainerId minimum containerId for filter
+ * @param maxContainerId maximum containerId for filter
* @param limit The total records to return
* @return List of unhealthy containers.
*/
public List<UnhealthyContainers> getUnhealthyContainers(
- UnHealthyContainerStates state, int offset, int limit) {
+ UnHealthyContainerStates state, Long minContainerId, Optional<Long> maxContainerId, int limit) {
DSLContext dslContext = containerSchemaDefinition.getDSLContext();
SelectQuery<Record> query = dslContext.selectQuery();
query.addFrom(UNHEALTHY_CONTAINERS);
+ Condition containerCondition;
+ OrderField[] orderField;
+ if (maxContainerId.isPresent() && maxContainerId.get() > 0) {
+ containerCondition = UNHEALTHY_CONTAINERS.CONTAINER_ID.lessThan(maxContainerId.get());
+ orderField = new OrderField[]{UNHEALTHY_CONTAINERS.CONTAINER_ID.desc(),
+ UNHEALTHY_CONTAINERS.CONTAINER_STATE.asc()};
+ } else {
+ containerCondition = UNHEALTHY_CONTAINERS.CONTAINER_ID.greaterThan(minContainerId);
+ orderField = new OrderField[]{UNHEALTHY_CONTAINERS.CONTAINER_ID.asc(),
+ UNHEALTHY_CONTAINERS.CONTAINER_STATE.asc()};
+ }
if (state != null) {
if (state.equals(ALL_REPLICAS_BAD)) {
- query.addConditions(UNHEALTHY_CONTAINERS.CONTAINER_STATE
- .eq(UNDER_REPLICATED.toString()));
+ query.addConditions(containerCondition.and(UNHEALTHY_CONTAINERS.CONTAINER_STATE
+ .eq(UNDER_REPLICATED.toString())));
query.addConditions(UNHEALTHY_CONTAINERS.ACTUAL_REPLICA_COUNT.eq(0));
} else {
- query.addConditions(
- UNHEALTHY_CONTAINERS.CONTAINER_STATE.eq(state.toString()));
+ query.addConditions(containerCondition.and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.eq(state.toString())));
}
}
- query.addOrderBy(UNHEALTHY_CONTAINERS.CONTAINER_ID.asc(),
- UNHEALTHY_CONTAINERS.CONTAINER_STATE.asc());
- query.addOffset(offset);
+
+ query.addOrderBy(orderField);
query.addLimit(limit);
- return query.fetchInto(UnhealthyContainers.class);
+ return query.fetchInto(UnhealthyContainers.class).stream()
+ .sorted(Comparator.comparingLong(UnhealthyContainers::getContainerId))
Review Comment:
Will the query not fetch the records already sorted ? This can break pagination because the last element returned won't necessarily match the expected prevLastKey for the next page.
Suppose the UI requests Page 1 and the API returns:
`[Container ID: 10, Container ID: 9, Container ID: 8] (SQL Order: DESC)`
You can incorrectly reorders it to:
`[Container ID: 8, Container ID: 9, Container ID: 10] (After Sorting)`
Now, `firstKey = 8 `and `lastKey = 10` instead of the correct `firstKey = 10` and `lastKey = 8.`
When the UI requests Page 2 using prevLastKey=8, the API will return overlapping or incorrect results.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/UnhealthyContainersResponse.java:
##########
@@ -49,6 +49,21 @@ public class UnhealthyContainersResponse {
@JsonProperty("misReplicatedCount")
private long misReplicatedCount = 0;
+ /**
+ * .
+ */
+ @JsonProperty("firstKey")
+ private long firstKey = 0;
+
+
+ /**
+ * Total count of mis-replicated containers.
+ */
+ @JsonProperty("lastKey")
+ private long lastKey = 0;
Review Comment:
We can rename this to lastContainerId of page.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -67,32 +72,43 @@ public ContainerHealthSchemaManager(
* matching the given state will be returned.
* @param state Return only containers in this state, or all containers if
* null
- * @param offset The starting record to return in the result set. The first
- * record is at zero.
+ * @param minContainerId minimum containerId for filter
+ * @param maxContainerId maximum containerId for filter
* @param limit The total records to return
* @return List of unhealthy containers.
*/
public List<UnhealthyContainers> getUnhealthyContainers(
- UnHealthyContainerStates state, int offset, int limit) {
+ UnHealthyContainerStates state, Long minContainerId, Optional<Long> maxContainerId, int limit) {
DSLContext dslContext = containerSchemaDefinition.getDSLContext();
SelectQuery<Record> query = dslContext.selectQuery();
query.addFrom(UNHEALTHY_CONTAINERS);
+ Condition containerCondition;
+ OrderField[] orderField;
+ if (maxContainerId.isPresent() && maxContainerId.get() > 0) {
+ containerCondition = UNHEALTHY_CONTAINERS.CONTAINER_ID.lessThan(maxContainerId.get());
+ orderField = new OrderField[]{UNHEALTHY_CONTAINERS.CONTAINER_ID.desc(),
+ UNHEALTHY_CONTAINERS.CONTAINER_STATE.asc()};
+ } else {
+ containerCondition = UNHEALTHY_CONTAINERS.CONTAINER_ID.greaterThan(minContainerId);
+ orderField = new OrderField[]{UNHEALTHY_CONTAINERS.CONTAINER_ID.asc(),
+ UNHEALTHY_CONTAINERS.CONTAINER_STATE.asc()};
+ }
if (state != null) {
if (state.equals(ALL_REPLICAS_BAD)) {
- query.addConditions(UNHEALTHY_CONTAINERS.CONTAINER_STATE
- .eq(UNDER_REPLICATED.toString()));
+ query.addConditions(containerCondition.and(UNHEALTHY_CONTAINERS.CONTAINER_STATE
+ .eq(UNDER_REPLICATED.toString())));
query.addConditions(UNHEALTHY_CONTAINERS.ACTUAL_REPLICA_COUNT.eq(0));
} else {
- query.addConditions(
- UNHEALTHY_CONTAINERS.CONTAINER_STATE.eq(state.toString()));
+ query.addConditions(containerCondition.and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.eq(state.toString())));
Review Comment:
We can add `EMPTY_MISSING` and `NEGATIVE_SIZE` containers in query itself not to include instead of excluding later. This ensures that pagination limits remain consistent.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -67,32 +72,43 @@ public ContainerHealthSchemaManager(
* matching the given state will be returned.
* @param state Return only containers in this state, or all containers if
* null
- * @param offset The starting record to return in the result set. The first
- * record is at zero.
+ * @param minContainerId minimum containerId for filter
+ * @param maxContainerId maximum containerId for filter
* @param limit The total records to return
* @return List of unhealthy containers.
*/
public List<UnhealthyContainers> getUnhealthyContainers(
- UnHealthyContainerStates state, int offset, int limit) {
+ UnHealthyContainerStates state, Long minContainerId, Optional<Long> maxContainerId, int limit) {
Review Comment:
If limit is set but the database has fewer results than expected, ensure that the UI or client application understands when it has reached the last page (i.e., no more results). Can we add an extra field in response something like below:
`response.setHasMore(unhealthyMeta.size() == limit);`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12708. Fix Unhealthy Containers API for pagination [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8174:
URL: https://github.com/apache/ozone/pull/8174#issuecomment-2901498553
/pending review comments, conflict
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12708. Fix Unhealthy Containers API for pagination [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #8174: HDDS-12708. Fix Unhealthy Containers API for pagination
URL: https://github.com/apache/ozone/pull/8174
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org