Copilot commented on code in PR #9399:
URL: https://github.com/apache/ozone/pull/9399#discussion_r2819784639
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -521,15 +563,31 @@ SnapshotDiffReportOzone createPageResponse(
final String bucketName,
final String fromSnapshotName,
final String toSnapshotName,
- final int index,
+ final String index,
final int pageSize
) throws IOException {
- if (index < 0 || index > snapDiffJob.getTotalDiffEntries()
- || pageSize <= 0) {
+ if (!isBlank(index)) {
+
DIFF_TYPE_STRING_MAP.values().stream().filter(index::startsWith).findFirst()
+ .orElseThrow(() -> new IOException("Token " + index + " has invalid
prefix. Valid prefixes: "
+ +
DIFF_TYPE_STRING_MAP.values().stream().map(String::valueOf).collect(Collectors.joining(","))));
+ }
+
+ int idx;
+ if (isBlank(index)) {
+ idx = 0;
+ } else {
+ try {
+ idx = Integer.parseInt(index.substring(1));
+ } catch (NumberFormatException e) {
+ throw new IOException("Token " + index + " has invalid numeric part: "
+
+ index.substring(1) + ". It should be a valid integer.", e);
+ }
+ }
+ if (idx < 0 || idx > snapDiffJob.getTotalDiffEntries() || pageSize <= 0) {
throw new IOException(String.format(
"Index (given: %d) should be a number >= 0 and < totalDiffEntries: "
+
"%d. Page size (given: %d) should be a positive number > 0.",
- index, snapDiffJob.getTotalDiffEntries(), pageSize));
+ idx, snapDiffJob.getTotalDiffEntries(), pageSize));
Review Comment:
The validation allows `idx == snapDiffJob.getTotalDiffEntries()`, which is
an invalid starting index. If a job has `totalDiffEntries` entries, the valid
indices are from 0 to `totalDiffEntries - 1`. Allowing `idx ==
totalDiffEntries` would result in an empty page, and if this happens to be the
last page, the integrity check on line 650 would fail because `pageLastKey`
would be an empty string (from the initialization on line 620) but
`diffJob.getLargestEntryKey()` would not be empty. The condition should be `idx
>= snapDiffJob.getTotalDiffEntries()` to properly validate the index range.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -588,16 +644,12 @@ Pair<List<DiffReportEntry>, String> createPageResponse(
*/
@VisibleForTesting
void checkReportsIntegrity(final SnapshotDiffJob diffJob,
- final int pageStartIdx,
- final int numberOfEntriesInPage)
- throws IOException {
- if ((pageStartIdx >= diffJob.getTotalDiffEntries() &&
- numberOfEntriesInPage != 0) || (pageStartIdx <
- diffJob.getTotalDiffEntries() && numberOfEntriesInPage == 0)) {
- LOG.error("Expected TotalDiffEntries: {} but found " +
- "TotalDiffEntries: {}",
- diffJob.getTotalDiffEntries(),
- pageStartIdx + numberOfEntriesInPage);
+ final String largestPageIndex,
+ boolean lastPage) throws IOException {
+ // For last page check last entry returned if the largest entry key equals
the largest key stored in the job entry.
+ if (lastPage && (diffJob.getLargestEntryKey() == null ||
!diffJob.getLargestEntryKey().equals(largestPageIndex))) {
+ LOG.error("Expected last entry: {} but found " +
+ "Largest Page entry: {}", diffJob.getLargestEntryKey(),
largestPageIndex);
Review Comment:
The parameter name `largestPageIndex` is misleading because it contains the
full key (e.g., `jobId-DELIMITER-100000000000000000005`), not just the index
portion. The parameter should be renamed to something more descriptive like
`pageLastKey` or `largestPageKey` to improve code readability and avoid
confusion.
```suggestion
final String pageLastKey,
boolean lastPage) throws IOException {
// For last page check last entry returned if the largest entry key
equals the largest key stored in the job entry.
if (lastPage && (diffJob.getLargestEntryKey() == null ||
!diffJob.getLargestEntryKey().equals(pageLastKey))) {
LOG.error("Expected last entry: {} but found " +
"Largest Page entry: {}", diffJob.getLargestEntryKey(),
pageLastKey);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
