You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ArafatKhan2198 (via GitHub)" <gi...@apache.org> on 2023/05/17 13:17:22 UTC
[GitHub] [ozone] ArafatKhan2198 opened a new pull request, #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
ArafatKhan2198 opened a new pull request, #4724:
URL: https://github.com/apache/ozone/pull/4724
## What changes were proposed in this pull request?
The `getContainers()` method in the container Endpoint has been modified to exclude the container specified by the `prevKey` parameter from the list of containers in the `ContainerResponse`.
The getContainers() method changes ensure that:
1. **Skipping the container with the same ID as prevKey**: The container stream is filtered to exclude the container matching prevKey, ensuring it is not included in the results.
2. **Increasing the limit by 1**: If prevKey is greater than zero, the limit is incremented by 1. This fetches an additional container after excluding the container with ID prevKey.
3. **Providing the last container ID**: The last container ID is determined based on the fetched container metadata. If no containers were fetched, last container ID is set to prevKey. Otherwise, it is obtained from the ID of the last container in the list. This information enables pagination and using the last container ID as the new prevKey value for subsequent requests.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8502
## How was this patch tested?
Modified the Existing Unit Tests to test out the imrpovements
--
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
[GitHub] [ozone] ArafatKhan2198 commented on pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on PR #4724:
URL: https://github.com/apache/ozone/pull/4724#issuecomment-1551381069
@devmadhuu @dombizita @krishnaasawa1 Can you please take a look.
--
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
[GitHub] [ozone] devmadhuu commented on pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #4724:
URL: https://github.com/apache/ozone/pull/4724#issuecomment-1556517498
Thanks for updating the patch @ArafatKhan2198 . LGTM +1
--
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
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1201954000
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,9 +140,12 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+
long containersCount;
- Collection<ContainerMetadata> containerMetaDataList =
- containerManager.getContainers(ContainerID.valueOf(prevKey), limit)
+ List<ContainerMetadata> containerMetaDataList =
+ // Get the containers starting from the prevKey+1 which will skip the
+ // container having prevKey ID
+ containerManager.getContainers(ContainerID.valueOf(prevKey + 1), limit)
Review Comment:
No, currently we have no UI components that are utilizing this specific endpoints alone `http://localhost:9888/api/v1/containers/` but we do have UI components that utilize this UI path such as
`http://localhost:9888/api/v1/containers/unhealthy` but the changes that we have made does not impact them.
--
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
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1198580365
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,10 +140,18 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+ if (prevKey > 0) {
+ // Increase the limit by 1 to fetch one additional container
+ // since we are excluding the container with the same ID as prevKey
+ limit = limit + 1;
Review Comment:
My Initial thought for raising the limit was that we might encounter non-sequential container ID's
When we sort container IDs in ascending order, we expect to have a sequence of consecutive numbers. For example 1, 2, 3, 4, 5, 6 would represent a proper sequence where each number follows the previous one. However, it's important to note that even though we sort the container IDs, it doesn't guarantee a fixed sequence.
In some cases, we may encounter container IDs that are not in a consecutive or sequential order. For instance, we might have container IDs like 1, 3, 4, 10, 23, 99, where there are gaps or missing numbers between the IDs. This can occur due to various reasons such as containers being deleted or missing. These missing or deleted containers result in gaps within the sequence of container IDs.
But after checking out the code in the [getContainers()](https://github.com/apache/ozone/blob/7d1aaef989f3749014c870000c84a6645d6fb43b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java#LL144C1-L146C1) it turns out even if we provide a containerId that does not exist it will still filter them out and start with a container ID that exists. So I believe the code changees suggested by @ashishkumar50 will work out fine.
--
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
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1200897847
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/ContainersResponse.java:
##########
@@ -60,16 +61,24 @@ public static class ContainersResponseData {
@JsonProperty("totalCount")
private long totalCount;
+ /**
+ * prevKey will be the last key of the previous page.
+ */
+ @JsonProperty("prevKey")
+ private long prevKey;
+
Review Comment:
Thanks for the comment @dombizita !
There are two reasons I chose to use the name `prevKey` :-
1. The response object's `prevKey` will be reused by the same API as the previous key for retrieving the next set of records, enabling pagination. Since the query parameter in the ContainerEndpoint is also named "prevKey," I decided to maintain consistency and keep the name as "prevKey."
2. `prevKey` is also used as the name in other APIs of Recon for pagination. By maintaining consistent naming conventions across the codebase, we can prevent confusion among developers.
Overall, these reasons support my decision to keep the variable named as "prevKey." Do you think we should change it to `lastContainerID` ?
--
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
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1196990567
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java:
##########
@@ -629,19 +642,20 @@ public void testGetContainersWithPrevKey()
ContainersResponse.ContainersResponseData data =
responseObject.getContainersResponseData();
- // Ensure that the total count of containers is 4
- assertEquals(4, data.getTotalCount());
+ // Ensure that the total count of containers is 3 as containers having
+ // ID's 1,2, will be skipped and the next 3 containers will be returned
+ assertEquals(3, data.getTotalCount());
Review Comment:
Thanks for the comment @devmadhuu
I have added assertions for non-sequential container Ids also
--
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
[GitHub] [ozone] ashishkumar50 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1197424558
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,10 +140,18 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+ if (prevKey > 0) {
+ // Increase the limit by 1 to fetch one additional container
+ // since we are excluding the container with the same ID as prevKey
+ limit = limit + 1;
Review Comment:
We can avoid increasing limit, as the getContainers() return results in sorted order, instead we can increase prevKey by 1 here as anyway we want to discard prevKey container info.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,10 +140,18 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+ if (prevKey > 0) {
+ // Increase the limit by 1 to fetch one additional container
+ // since we are excluding the container with the same ID as prevKey
+ limit = limit + 1;
+ }
+
long containersCount;
- Collection<ContainerMetadata> containerMetaDataList =
+ List<ContainerMetadata> containerMetaDataList =
containerManager.getContainers(ContainerID.valueOf(prevKey), limit)
.stream()
+ // Exclude the container with the same ID as prevKey
+ .filter(container -> container.getContainerID() != prevKey)
Review Comment:
By increasing prevKey by one in the before step we don't need to do filter here as the results won't contain prevKey container info. And by doing this we get actual number of container information and not one extra container detail.
--
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
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1198581576
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,10 +140,18 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+ if (prevKey > 0) {
+ // Increase the limit by 1 to fetch one additional container
+ // since we are excluding the container with the same ID as prevKey
+ limit = limit + 1;
+ }
+
long containersCount;
- Collection<ContainerMetadata> containerMetaDataList =
+ List<ContainerMetadata> containerMetaDataList =
containerManager.getContainers(ContainerID.valueOf(prevKey), limit)
.stream()
+ // Exclude the container with the same ID as prevKey
+ .filter(container -> container.getContainerID() != prevKey)
Review Comment:
Done thanks for the suggestion !!
--
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
[GitHub] [ozone] dombizita commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1201900650
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,9 +140,12 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+
long containersCount;
- Collection<ContainerMetadata> containerMetaDataList =
- containerManager.getContainers(ContainerID.valueOf(prevKey), limit)
+ List<ContainerMetadata> containerMetaDataList =
+ // Get the containers starting from the prevKey+1 which will skip the
+ // container having prevKey ID
+ containerManager.getContainers(ContainerID.valueOf(prevKey + 1), limit)
Review Comment:
one small question: this behaviour change won't have effect in the UI? or other places this method is used?
or it was working incorrectly before so it will work correctly now?
--
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
[GitHub] [ozone] dombizita commented on pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on PR #4724:
URL: https://github.com/apache/ozone/pull/4724#issuecomment-1558963733
thanks for the patch @ArafatKhan2198! thanks for the review @ashishkumar50 @devmadhuu!
--
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
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1198582272
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,10 +140,18 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+ if (prevKey > 0) {
Review Comment:
I have made the changes thanks !!
--
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
[GitHub] [ozone] dombizita commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1201902024
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/ContainersResponse.java:
##########
@@ -60,16 +61,24 @@ public static class ContainersResponseData {
@JsonProperty("totalCount")
private long totalCount;
+ /**
+ * prevKey will be the last key of the previous page.
+ */
+ @JsonProperty("prevKey")
+ private long prevKey;
+
Review Comment:
thanks for the explanation @ArafatKhan2198, it makes sense to leave it as it is!
--
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
[GitHub] [ozone] dombizita commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1201964057
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,9 +140,12 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+
long containersCount;
- Collection<ContainerMetadata> containerMetaDataList =
- containerManager.getContainers(ContainerID.valueOf(prevKey), limit)
+ List<ContainerMetadata> containerMetaDataList =
+ // Get the containers starting from the prevKey+1 which will skip the
+ // container having prevKey ID
+ containerManager.getContainers(ContainerID.valueOf(prevKey + 1), limit)
Review Comment:
okay, thanks for checking it @ArafatKhan2198!
--
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
[GitHub] [ozone] dombizita merged pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita merged PR #4724:
URL: https://github.com/apache/ozone/pull/4724
--
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
[GitHub] [ozone] krishnaasawa1 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "krishnaasawa1 (via GitHub)" <gi...@apache.org>.
krishnaasawa1 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1197881330
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,10 +140,18 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+ if (prevKey > 0) {
Review Comment:
prevkey will be 0 at the beginning . Comment of this method it self define "prevKey the containerID after which results are returned.
* start containerID, >=0"
if so this check is not required else even 1st call will not go thru.
prevKey < 0 is already taken care above.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,10 +140,18 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+ if (prevKey > 0) {
+ // Increase the limit by 1 to fetch one additional container
+ // since we are excluding the container with the same ID as prevKey
+ limit = limit + 1;
Review Comment:
Make sense if this works , this will minimize the code change here.
--
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
[GitHub] [ozone] dombizita commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1200474174
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/ContainersResponse.java:
##########
@@ -60,16 +61,24 @@ public static class ContainersResponseData {
@JsonProperty("totalCount")
private long totalCount;
+ /**
+ * prevKey will be the last key of the previous page.
+ */
+ @JsonProperty("prevKey")
+ private long prevKey;
+
Review Comment:
in this class you reference it as `prevKey` but in the `ContainerEndpoint` when you are passing it you say `lastContainerID` (also in the jira description is says it this way)
https://github.com/apache/ozone/blob/d2c3b4aad073f9b28ee30f3c80cf12c6f4247326/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java#L161-L163
shouldn't we use `lastContainerID` here as well? `prevKey` seems a bit hard to understand at first, or maybe just document the change better.
let me know what you think!
--
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
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1198580658
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -141,10 +140,18 @@ public Response getContainers(
// Send back an empty response
return Response.status(Response.Status.NOT_ACCEPTABLE).build();
}
+ if (prevKey > 0) {
+ // Increase the limit by 1 to fetch one additional container
+ // since we are excluding the container with the same ID as prevKey
+ limit = limit + 1;
Review Comment:
I have ran the existing UT's for the new changes and they work out fine !!
--
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
[GitHub] [ozone] devmadhuu commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1196594522
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java:
##########
@@ -629,19 +642,20 @@ public void testGetContainersWithPrevKey()
ContainersResponse.ContainersResponseData data =
responseObject.getContainersResponseData();
- // Ensure that the total count of containers is 4
- assertEquals(4, data.getTotalCount());
+ // Ensure that the total count of containers is 3 as containers having
+ // ID's 1,2, will be skipped and the next 3 containers will be returned
+ assertEquals(3, data.getTotalCount());
Review Comment:
can you pls assert prevKey also here ?
--
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
[GitHub] [ozone] devmadhuu commented on pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #4724:
URL: https://github.com/apache/ozone/pull/4724#issuecomment-1551486642
@ArafatKhan2198 thanks for working on this patch, can you also pls add a test case with adding non-sequential container Ids and do all assertions ?
--
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
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4724: HDDS-8502. Recon - getContainers API is not giving expected response with prevKey.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4724:
URL: https://github.com/apache/ozone/pull/4724#discussion_r1200897847
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/ContainersResponse.java:
##########
@@ -60,16 +61,24 @@ public static class ContainersResponseData {
@JsonProperty("totalCount")
private long totalCount;
+ /**
+ * prevKey will be the last key of the previous page.
+ */
+ @JsonProperty("prevKey")
+ private long prevKey;
+
Review Comment:
Thanks for the comment @dombizita !
There are two reasons I chose to use the name `prevKey` :-
1. The response object's `prevKey` will be reused by the same API as the previous key for retrieving the next set of records, enabling pagination. Since the query parameter in the ContainerEndpoint is also named "prevKey," I decided to maintain consistency and keep the name as "prevKey."
2. `prevKey` is also used as the name in other APIs of Recon for pagination. By maintaining consistent naming conventions across the codebase, we can prevent confusion among developers.
Overall, these reasons support my decision to keep the variable named as "prevKey."
--
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