Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Azure batch deletes #1365

Merged
merged 10 commits into from
Feb 4, 2020
Merged

Azure batch deletes #1365

merged 10 commits into from
Feb 4, 2020

Conversation

lightningrob
Copy link
Contributor

No description provided.

try {
statusCode = response.getStatusCode();
} catch (BlobStorageException bex) {
statusCode = bex.getStatusCode();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the statuscode returned by exception be one of OK, ACCEPTED, NOT_FOUND or GONE? If not, then maybe we can move the try catch block to include switch as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They throw exception if the http status is not the "expected" value, which in this case is Accepted.

@codecov-io
Copy link

codecov-io commented Jan 31, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@805a993). Click here to learn what that means.
The diff coverage is 91.17%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1365   +/-   ##
=========================================
  Coverage          ?   71.99%           
  Complexity        ?     6759           
=========================================
  Files             ?      486           
  Lines             ?    38378           
  Branches          ?     4880           
=========================================
  Hits              ?    27629           
  Misses            ?     9415           
  Partials          ?     1334
Impacted Files Coverage Δ Complexity Δ
...b.ambry.clustermap/StaticClusterAgentsFactory.java 31.57% <0%> (ø) 4 <0> (?)
...ithub.ambry.cloud/azure/AzureCloudDestination.java 72.98% <100%> (ø) 30 <0> (?)
.../main/java/com.github.ambry/store/MessageInfo.java 89.41% <100%> (ø) 25 <1> (?)
...ls/src/main/java/com.github.ambry.utils/Utils.java 78.46% <100%> (ø) 124 <5> (?)
...essageformat/UndeleteMessageFormatInputStream.java 100% <100%> (ø) 1 <1> (?)
...b.ambry.clustermap/SimpleClusterChangeHandler.java 91.78% <100%> (ø) 50 <0> (?)
...java/com.github.ambry/config/ClusterMapConfig.java 100% <100%> (ø) 3 <0> (?)
...hub.ambry/clustermap/StateTransitionException.java 100% <100%> (ø) 2 <0> (?)
...a/com.github.ambry.clustermap/ClusterMapUtils.java 93.86% <100%> (ø) 46 <4> (?)
...java/com.github.ambry.router/GetBlobOperation.java 91.52% <100%> (ø) 44 <0> (?)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 805a993...06e5b3f. Read the comment docs.

Copy link
Contributor

@cgtz cgtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after comments are addressed

throw new IllegalArgumentException("Invalid batchSize: " + batchSize);
}
List<List<T>> partitionedLists = new ArrayList<>();
for (int j = 0; j < inputList.size() / batchSize + 1; j++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this loop be simplified to:

for (int start = 0; start < inputList.size(); start += batchSize) {
  int end = Math.min(start + batchSize, inputList.size());
  partitionedLists.add(inputList.subList(start, end));
}

I may be missing an edge case here though

Copy link
Contributor

@cgtz cgtz Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I was thinking about this partition thing and it seems that we don't really need a full list and just need an iterable that we can do a for loop on. Something like this could work, I think:

  public static <T> Iterable<List<T>> partition(List<T> list, int batchSize) {
    return () -> new Iterator<List<T>>() {
      int nextStart = 0;

      @Override
      public boolean hasNext() {
        return nextStart < list.size();
      }

      @Override
      public List<T> next() {
        if (!hasNext()) {
          throw new NoSuchElementException();
        }
        int start = nextStart;
        nextStart += batchSize;
        return list.subList(start, Math.min(start + batchSize, list.size()));
      }
    };
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is a big improvement. I prefer to mimic the guava utility. Once we upgrade to a newer JDK, we can probably retire the utility and use the Collections one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the simplification you suggested.

@cgtz cgtz merged commit db13bc3 into linkedin:master Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants