[GitHub] [hadoop] avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add support for 'start' query param to containers…
avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add
support for 'start' query param to containers…
URL: https://github.com/apache/hadoop/pull/987#discussion_r298426673
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
##
@@ -128,23 +128,65 @@ public Integer getCountForForContainerKeyPrefix(
}
/**
- * Use the DB's prefix seek iterator to start the scan from the given
- * container ID prefix.
+ * Get key prefixes for the given container ID.
*
* @param containerId the given containerId.
* @return Map of (Key-Prefix,Count of Keys).
*/
@Override
public Map getKeyPrefixesForContainer(
long containerId) throws IOException {
+// set the default startKeyPrefix to empty string
+return getKeyPrefixesForContainer(containerId, StringUtils.EMPTY);
+ }
+
+ /**
+ * Use the DB's prefix seek iterator to start the scan from the given
+ * container ID and prev key prefix. The prev key prefix is skipped from
+ * the result.
+ *
+ * @param containerId the given containerId.
+ * @param prevKeyPrefix the given key prefix to start the scan from.
+ * @return Map of (Key-Prefix,Count of Keys).
+ */
+ @Override
+ public Map getKeyPrefixesForContainer(
+ long containerId, String prevKeyPrefix) throws IOException {
Map prefixes = new LinkedHashMap<>();
TableIterator> containerIterator = containerKeyTable.iterator();
-containerIterator.seek(new ContainerKeyPrefix(containerId));
+ContainerKeyPrefix seekKey;
+boolean skipPrevKey = false;
+if (StringUtils.isNotBlank(prevKeyPrefix)) {
+ skipPrevKey = true;
+ seekKey = new ContainerKeyPrefix(containerId, prevKeyPrefix);
+} else {
+ seekKey = new ContainerKeyPrefix(containerId);
+}
+KeyValue seekKeyValue =
+containerIterator.seek(seekKey);
+
+// check if RocksDB was able to seek correctly to the given key prefix
+// if not, then return empty result
+// In case of an empty prevKeyPrefix, all the keys in the container are
+// returned
+if (seekKeyValue == null ||
+(StringUtils.isNotBlank(prevKeyPrefix) &&
Review comment:
The assumption is that, the previous key prefix being passed in is always
present in our Rocksdb. The UI container key table probably passes in only
valid key prefixes for pagination. But maybe the API can support an arbitrary
previous key prefix also. We can refine this in subsequent changes when the use
cases are more defined.
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] avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add support for 'start' query param to containers…
avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add
support for 'start' query param to containers…
URL: https://github.com/apache/hadoop/pull/987#discussion_r298427231
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
##
@@ -164,38 +206,49 @@ public Integer getCountForForContainerKeyPrefix(
return prefixes;
}
- /**
- * Get all the containers.
- *
- * @return Map of containerID -> containerMetadata.
- * @throws IOException
- */
- @Override
- public Map getContainers() throws IOException {
-// Set a negative limit to get all the containers.
-return getContainers(-1);
- }
-
/**
* Iterate the DB to construct a Map of containerID -> containerMetadata
- * only for the given limit.
+ * only for the given limit from the given start key. The start containerID
+ * is skipped from the result.
*
* Return all the containers if limit < 0.
*
+ * @param limit No of containers to get.
+ * @param prevKey containerID after which the list of containers are scanned.
* @return Map of containerID -> containerMetadata.
* @throws IOException
*/
@Override
- public Map getContainers(int limit)
+ public Map getContainers(int limit, long prevKey)
Review comment:
Can we name the argument previousContainer instead of 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.
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] avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add support for 'start' query param to containers…
avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add
support for 'start' query param to containers…
URL: https://github.com/apache/hadoop/pull/987#discussion_r298428634
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
##
@@ -164,38 +206,49 @@ public Integer getCountForForContainerKeyPrefix(
return prefixes;
}
- /**
- * Get all the containers.
- *
- * @return Map of containerID -> containerMetadata.
- * @throws IOException
- */
- @Override
- public Map getContainers() throws IOException {
-// Set a negative limit to get all the containers.
-return getContainers(-1);
- }
-
/**
* Iterate the DB to construct a Map of containerID -> containerMetadata
- * only for the given limit.
+ * only for the given limit from the given start key. The start containerID
+ * is skipped from the result.
*
* Return all the containers if limit < 0.
*
+ * @param limit No of containers to get.
+ * @param prevKey containerID after which the list of containers are scanned.
* @return Map of containerID -> containerMetadata.
* @throws IOException
*/
@Override
- public Map getContainers(int limit)
+ public Map getContainers(int limit, long prevKey)
throws IOException {
Map containers = new LinkedHashMap<>();
TableIterator> containerIterator = containerKeyTable.iterator();
+boolean skipPrevKey = false;
+ContainerKeyPrefix seekKey;
+if (prevKey > 0L) {
+ skipPrevKey = true;
Review comment:
I believe we don't need this skipPrevKey flag. If the value of prevKey < 0,
then the method will return at Line 238. Also, instead of iterating the entire
key space of the prevKey container, can we do something like setting seek key
to prevKey+1. By that way, we will always pick the next container.
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] avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add support for 'start' query param to containers…
avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add
support for 'start' query param to containers…
URL: https://github.com/apache/hadoop/pull/987#discussion_r296376203
##
File path:
hadoop-ozone/ozone-recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerKeyService.java
##
@@ -227,20 +227,38 @@ public void testGetKeysForContainer() {
assertEquals(blockIds.get(0L).iterator().next().getLocalID(), 103);
assertEquals(blockIds.get(1L).iterator().next().getLocalID(), 104);
-response = containerKeyService.getKeysForContainer(3L, -1);
+response = containerKeyService.getKeysForContainer(3L, -1, "");
keyMetadataList = (Collection) response.getEntity();
assertTrue(keyMetadataList.isEmpty());
// test if limit works as expected
-response = containerKeyService.getKeysForContainer(1L, 1);
+response = containerKeyService.getKeysForContainer(1L, 1, "");
keyMetadataList = (Collection) response.getEntity();
assertEquals(keyMetadataList.size(), 1);
+
+// test if start param works as expected
+response = containerKeyService.getKeysForContainer(
+1L, -1, "/sampleVol/bucketOne/key_one");
Review comment:
Are we testing all negative cases - Limit not being specified, Start key
prefix not found ? Maybe we can split this UT into multiple ones.
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] avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add support for 'start' query param to containers…
avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add support for 'start' query param to containers… URL: https://github.com/apache/hadoop/pull/987#discussion_r296032651 ## File path: hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/ContainerDBServiceProvider.java ## @@ -85,7 +86,8 @@ Integer getCountForForContainerKeyPrefix( * @return Map of containerID -> containerMetadata. * @throws IOException */ - Map getContainers(int limit) throws IOException; + Map getContainers(int limit, long start) Review comment: According to the implementation, the start key will be skipped if present in the seek. This is to support a pagination kind of API. Maybe, we can change the param name to reflect this. Something like previous instead of start? 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] avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add support for 'start' query param to containers…
avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add
support for 'start' query param to containers…
URL: https://github.com/apache/hadoop/pull/987#discussion_r296032332
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
##
@@ -164,38 +194,42 @@ public Integer getCountForForContainerKeyPrefix(
return prefixes;
}
- /**
- * Get all the containers.
- *
- * @return Map of containerID -> containerMetadata.
- * @throws IOException
- */
- @Override
- public Map getContainers() throws IOException {
-// Set a negative limit to get all the containers.
-return getContainers(-1);
Review comment:
Can we name the -1 as something like Container.ALL?
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] avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add support for 'start' query param to containers…
avijayanhwx commented on a change in pull request #987: HDDS-1685. Recon: Add
support for 'start' query param to containers…
URL: https://github.com/apache/hadoop/pull/987#discussion_r296030739
##
File path:
hadoop-ozone/ozone-recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
##
@@ -128,23 +128,53 @@ public Integer getCountForForContainerKeyPrefix(
}
/**
- * Use the DB's prefix seek iterator to start the scan from the given
- * container ID prefix.
+ * Get key prefixes for the given container ID.
*
* @param containerId the given containerId.
* @return Map of (Key-Prefix,Count of Keys).
*/
@Override
public Map getKeyPrefixesForContainer(
long containerId) throws IOException {
+// set the default startKeyPrefix to empty string
+return getKeyPrefixesForContainer(containerId, "");
Review comment:
Maybe we can use StringUtils.EMPTY.
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
