[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294601234
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/ContainerDBServiceProvider.java
##
@@ -70,13 +70,23 @@ Integer getCountForForContainerKeyPrefix(
throws IOException;
/**
- * Get a Map of containerID, containerMetadata of all Containers.
+ * Get a Map of containerID, containerMetadata of all the Containers.
*
* @return Map of containerID -> containerMetadata.
* @throws IOException
*/
Map getContainers() throws IOException;
Review comment:
We can remove this method, in your further jira for considering the start
parameter.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294600693
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
##
@@ -181,6 +182,12 @@ public Integer getCountForForContainerKeyPrefix(
Long containerID = keyValue.getKey().getContainerId();
Integer numberOfKeys = keyValue.getValue();
+ // break the loop if limit has been reached
+ // and one more new entity needs to be added to the containers map
+ if (containers.size() == limit && !containers.containsKey(containerID)) {
Review comment:
Got it..
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294586933
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/ContainerDBServiceProvider.java
##
@@ -70,13 +70,23 @@ Integer getCountForForContainerKeyPrefix(
throws IOException;
/**
- * Get a Map of containerID, containerMetadata of all Containers.
+ * Get a Map of containerID, containerMetadata of all the Containers.
*
* @return Map of containerID -> containerMetadata.
* @throws IOException
*/
Map getContainers() throws IOException;
Review comment:
Minor: I see the only new method being used.
Old method is not being used from the API. I have assumed this will be
called from service.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294586933
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/ContainerDBServiceProvider.java
##
@@ -70,13 +70,23 @@ Integer getCountForForContainerKeyPrefix(
throws IOException;
/**
- * Get a Map of containerID, containerMetadata of all Containers.
+ * Get a Map of containerID, containerMetadata of all the Containers.
*
* @return Map of containerID -> containerMetadata.
* @throws IOException
*/
Map getContainers() throws IOException;
Review comment:
Minor: I see the only new method being used.
Old method is not being used from the API.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294536580
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerKeyService.java
##
@@ -72,10 +74,11 @@
* @return {@link Response}
*/
@GET
- public Response getContainers() {
+ public Response getContainers(
+ @DefaultValue("-1") @QueryParam("limit") int limit) {
Map containersMap;
try {
- containersMap = containerDBServiceProvider.getContainers();
+ containersMap = containerDBServiceProvider.getContainers(limit);
Review comment:
Okay. Then I feel we can have 2 methods, getContainers(), getContainers(int
limit)
In this way, we don't need any special considerations in underlying
implementation
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294527606
##
File path:
hadoop-ozone/ozone-recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerKeyService.java
##
@@ -200,56 +201,67 @@ protected void configure() {
@Test
public void testGetKeysForContainer() {
-Response response = containerKeyService.getKeysForContainer(1L);
+Response response = containerKeyService.getKeysForContainer(1L, 2);
Collection keyMetadataList =
(Collection) response.getEntity();
-assertTrue(keyMetadataList.size() == 2);
+assertEquals(keyMetadataList.size(), 2);
Review comment:
Can we add tests with default value of limit 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294527606
##
File path:
hadoop-ozone/ozone-recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerKeyService.java
##
@@ -200,56 +201,67 @@ protected void configure() {
@Test
public void testGetKeysForContainer() {
-Response response = containerKeyService.getKeysForContainer(1L);
+Response response = containerKeyService.getKeysForContainer(1L, 2);
Collection keyMetadataList =
(Collection) response.getEntity();
-assertTrue(keyMetadataList.size() == 2);
+assertEquals(keyMetadataList.size(), 2);
Review comment:
Can we add tests with default value of limit 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294527750
##
File path:
hadoop-ozone/ozone-recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerKeyService.java
##
@@ -200,56 +201,67 @@ protected void configure() {
@Test
public void testGetKeysForContainer() {
-Response response = containerKeyService.getKeysForContainer(1L);
+Response response = containerKeyService.getKeysForContainer(1L, 2);
Review comment:
Can we add tests with default value of limit 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294526542
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
##
@@ -181,6 +182,12 @@ public Integer getCountForForContainerKeyPrefix(
Long containerID = keyValue.getKey().getContainerId();
Integer numberOfKeys = keyValue.getValue();
+ // break the loop if limit has been reached
+ // and one more new entity needs to be added to the containers map
+ if (containers.size() == limit && !containers.containsKey(containerID)) {
Review comment:
Not understood the reason for the 2nd condition.
And this method can be called with limit -1, Can you update java doc with
description, what happens in that case. And do we need to add any check for
other negative values for this 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294526542
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
##
@@ -181,6 +182,12 @@ public Integer getCountForForContainerKeyPrefix(
Long containerID = keyValue.getKey().getContainerId();
Integer numberOfKeys = keyValue.getValue();
+ // break the loop if limit has been reached
+ // and one more new entity needs to be added to the containers map
+ if (containers.size() == limit && !containers.containsKey(containerID)) {
Review comment:
Not understood the reason for the 2nd condition.
And this method can be called with limit -1, Can you update java doc with
description, what happens in that case.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294525907
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerKeyService.java
##
@@ -72,10 +74,11 @@
* @return {@link Response}
*/
@GET
- public Response getContainers() {
+ public Response getContainers(
+ @DefaultValue("-1") @QueryParam("limit") int limit) {
Map containersMap;
try {
- containersMap = containerDBServiceProvider.getContainers();
+ containersMap = containerDBServiceProvider.getContainers(limit);
Review comment:
Default value of the query param limit is -1.
So, in the case of -1, it behaves like normal, it lists all the containers?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r294525907
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerKeyService.java
##
@@ -72,10 +74,11 @@
* @return {@link Response}
*/
@GET
- public Response getContainers() {
+ public Response getContainers(
+ @DefaultValue("-1") @QueryParam("limit") int limit) {
Map containersMap;
try {
- containersMap = containerDBServiceProvider.getContainers();
+ containersMap = containerDBServiceProvider.getContainers(limit);
Review comment:
Default value of the query param limit is -1.
So, in the case of -1, it behaves like normal, it lists all the containers.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r293584974
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerKeyService.java
##
@@ -72,10 +74,11 @@
* @return {@link Response}
*/
@GET
- public Response getContainers() {
+ public Response getContainers(
+ @DefaultValue("-1") @QueryParam("limit") int limit) {
Map containersMap;
try {
- containersMap = containerDBServiceProvider.getContainers();
+ containersMap = containerDBServiceProvider.getContainers(limit);
} catch (IOException ioEx) {
Review comment:
If we have support for continuation, in the next call we shall not require
to fetch limit + 50, we can fetch next 50.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r293522345
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerKeyService.java
##
@@ -92,8 +95,10 @@ public Response getContainers() {
*/
@GET
@Path("/{id}")
- public Response getKeysForContainer(@PathParam("id") Long containerId) {
-Map keyMetadataMap = new HashMap<>();
+ public Response getKeysForContainer(
+ @PathParam("id") Long containerId,
+ @DefaultValue("-1") @QueryParam("limit") int limit) {
Review comment:
Same as above.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit support to /api/containers and /api/containers/{id} endpoints
bharatviswa504 commented on a change in pull request #954: HDDS-1670. Add limit
support to /api/containers and /api/containers/{id} endpoints
URL: https://github.com/apache/hadoop/pull/954#discussion_r293522160
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerKeyService.java
##
@@ -72,10 +74,11 @@
* @return {@link Response}
*/
@GET
- public Response getContainers() {
+ public Response getContainers(
+ @DefaultValue("-1") @QueryParam("limit") int limit) {
Map containersMap;
try {
- containersMap = containerDBServiceProvider.getContainers();
+ containersMap = containerDBServiceProvider.getContainers(limit);
} catch (IOException ioEx) {
Review comment:
I see the API to limit.
But how we shall get the list of containers from the last returned result,
will be there be an API support for that?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
