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

Stitched blobs support different sized component blobs #1175

Merged
merged 15 commits into from
Jun 10, 2019

Conversation

dharju
Copy link
Contributor

@dharju dharju commented May 23, 2019

Changed stitched blobs to work with blobs with different data content sizes.

Note: Still need to add serializeMetadataContentV3 into BlobIdTransformer, but regular stitched blob operation changes are ready to be looked at

@dharju dharju requested review from cgtz and jsjtzyy May 23, 2019 11:28
@codecov-io
Copy link

codecov-io commented May 23, 2019

Codecov Report

Merging #1175 into master will increase coverage by 13.55%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #1175       +/-   ##
=============================================
+ Coverage     70.06%   83.62%   +13.55%     
+ Complexity     5378       57     -5321     
=============================================
  Files           428        6      -422     
  Lines         32791      348    -32443     
  Branches       4136       38     -4098     
=============================================
- Hits          22975      291    -22684     
+ Misses         8691       45     -8646     
+ Partials       1125       12     -1113
Impacted Files Coverage Δ Complexity Δ
...ava/com.github.ambry/config/PerformanceConfig.java
...main/java/com.github.ambry.cloud/CloudReplica.java
...m.github.ambry.replication/ReplicationMetrics.java
...ava/com.github.ambry/frontend/SecurityService.java
...b.ambry.cloud/CloudBlobCryptoAgentFactoryImpl.java
...ain/java/com.github.ambry/frontend/Operations.java
...n/java/com.github.ambry.clustermap/Datacenter.java
...github.ambry.messageformat/TtlUpdateSubRecord.java
...i/src/main/java/com.github.ambry/network/Port.java
...n/java/com.github.ambry.network/CompositeSend.java
... and 405 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 09f0d30...870c22e. Read the comment docs.

Copy link
Contributor

@jsjtzyy jsjtzyy left a comment

Choose a reason for hiding this comment

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

Some minor comments. Will take a look at tests soon.

*/
public static int getMetadataContentSize(int keySize, int numberOfKeys) {
return Version_Field_Size_In_Bytes + TOTAL_SIZE_FIELD_SIZE_IN_BYTES + NUM_OF_KEYS_FIELD_SIZE_IN_BYTES + (
numberOfKeys * keySize * SIZE_OF_BLOB_FIELD_IN_BYTES);
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I am wrong, I think here should be numberOfKeys * ( keySize + SIZE_OF_BLOB_FIELD_IN_BYTES )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, adding unit test to catch this

* Serialize a metadata content record.
* @param outputBuffer
* @param totalSize
* @param keysAndContentSizes
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you complete the java doc here?


/**
* Construct a {@link CompositeBlobInfo} object.
* @param keysAndContentSizes
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

* blob
* @param start inclusive starting byte index
* @param end inclusive ending byte index
* @return
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

*/
public List<StoreKeyAndSizeAndOffset> getStoreKeysInByteRange(long start, long end) {
if (end < start || start < 0L || end >= totalSize) {
throw new IllegalArgumentException("Bad input parameters, start="+start+" end="+end+" totalSize="+totalSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

format this file please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

int idx = Collections.binarySearch(offsets, start);
//binarySearch returns -(insertion point) - 1 if not an exact match, which points to the index of
//the first element greater than the key, or list.size() if all elements in the list are
// less than the specified key.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this description comes from Collections.binarySearch java doc, however, in current context, we'd better use target or start instead of key here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, changed to 'start'

//the first element greater than the key, or list.size() if all elements in the list are
// less than the specified key.
if (idx < 0) {
idx = -idx-2; //points to element with offset closest to but less than 'start'
Copy link
Contributor

Choose a reason for hiding this comment

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

Although it seems that idx won't be -1, it is worthwhile to add comment here to explain it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first offset will always be '0', so idx will never be -1 (it would only be -1 if 'start' was less than the first offset)

public static CompositeBlobInfo deserializeMetadataContentRecord(DataInputStream stream,
StoreKeyFactory storeKeyFactory) throws IOException {
List<Pair<StoreKey, Long>> keysAndContentSizes = new ArrayList<>();
stream.readLong(); //total size
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest passing the total size into CompositeBlobInfo constructor and compare it with the calculated total size (which is last in your code). If they don't match, throw an exception.

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 for adding the total size as a param to the CompositeBlobInfo ctor to just perform a check, but I will add the check here, and in serializeMetadataContentRecord

Copy link
Contributor

@jsjtzyy jsjtzyy left a comment

Choose a reason for hiding this comment

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

A few more comments. Feel free to drop some if I misunderstood the code logic.

List<StoreKey> orderedChunkIdList = new ArrayList<>(indexToChunkIds.values());
buf = MetadataContentSerDe.serializeMetadataContent(intermediateChunkSize, getBlobSize(), orderedChunkIdList);
List<Pair<StoreKey, Long>> orderedChunkIdList = indexToChunkIds.values().stream().collect(
Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

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

Format this file. Also looks like we can still use new ArrayList<>(indexToChunkIds.values()), any specific reason to use indexToChunkIds.values().stream().collect(Collectors.toList()) here?

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 don't think there's a specific reason, replaced

@@ -1619,7 +1623,8 @@ private void finalizeMetadataChunk() {
* @return a list of all of the successfully put chunk ids associated with this blob
*/
List<StoreKey> getSuccessfullyPutChunkIds() {
return new ArrayList<>(indexToChunkIds.values());
return indexToChunkIds.values().stream().map(b -> b.getFirst()).collect(
Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please format here. And maybe use method reference to simplify:
indexToChunkIds.values().stream().map(Pair::getFirst).collect(Collectors.toList())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@@ -937,9 +943,9 @@ protected ByteBuffer filterChunkToRange(ByteBuffer buf) {
buf.position(0);
buf.limit(0);
} else {
long startOffsetInThisChunk = chunkIndex == 0 ? resolvedByteRange.getStartOffset() % chunkSize : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder previously if chunkIndex > 0, why the startOffsetInThisChunk is 0 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it's relative to the chunk itself. The chunkIndexes only refer to chunks that are within a byte range. If the chunkIndex refers to a chunk beyond the first chunk, it will have to start at 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about the confusion around chunkIndex. Since we now have the offset variable, you could consider changing this to long startOffsetInThisChunk = Math.max(resolvedByteRange.getStartOffset() - offset, 0) if you prefer.

long endOffsetInThisChunk =
chunkIndex == (numChunksTotal - 1) ? (resolvedByteRange.getEndOffset() % chunkSize) + 1 : chunkSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

One more minor question: is endOffsetInThisChunk exclusive or inclusive?
Looks like it is exclusive, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exclusive since it's used in Buffer.limit(). I can rename it endOffsetInThisChunkExclusive

Copy link
Contributor

Choose a reason for hiding this comment

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

If you feel that it is cleaner, you could also express this as long endOffsetInThisChunkExclusive = Math.min(resolvedByteRange.getEndOffset() - offset + 1, chunkSize)

private BlobProperties serverBlobProperties;

/**
* Construct a FirstGetChunk and initialize it with the {@link BlobId} of the overall operation.
*/
FirstGetChunk() {
super(-1, blobId);
super(-1, new CompositeBlobInfo.StoreKeyAndSizeAndOffset(blobId, 0L, -1L));
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the chunkSize is initialized to -1L here and I don't see it gets populated again because you removed several codes regarding setting chunkSize. Correct me if I am wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, but that's ok since chunkSize is only used when calculating the end of data chunk for a byte range operation if that data chunk is not the last data chunk. For simple data blobs, the FirstChunk will always be the last data chunk so it's unused, and for metadata blobs the size of the FirstChunk is irrelevant.

@@ -1051,7 +1057,6 @@ protected void maybeProcessCallbacks() {
decryptCallbackResultInfo.result.getDecryptedUserMetadata().array());
}
totalSize = decryptCallbackResultInfo.result.getDecryptedBlobContent().remaining();
chunkSize = totalSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to previous comment. Where the chunkSize is populated? I notice that getKeysAndSizesAndOffsets contains chunk size info, however, note that filterChunkToRange method directly uses variable chunkSize, which might not be initialized. Please double check code there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we can discuss offline about the chunk size of a simple blob.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ok since chunkSize is only used when calculating the end of data chunk for a byte range operation if that data chunk is not the last data chunk. For simple data blobs, the FirstChunk will always be the last data chunk so it's unused, and for metadata blobs the size of the FirstChunk is irrelevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you choose to use min/max in filterChunkToRange, I think it will be required to set chunkSize appropriately on simple blobs

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.

Will continue to review

}
}


private final int chunkSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are chunkSize/keys still needed after introducing StoreKeyAndSizeAndOffset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chunk size would still be used for deserializing V2 metadata content and keys is used so that getKeys() call doesn't have to parse the StoreKeyAndSizeAndOffset object every time it is called

* POJO class for holding a store key, the size of the data content it refers to,
* and the offset of the data in the larger composite blob
*/
public static class StoreKeyAndSizeAndOffset {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more concise name that we could choose for this class? This name might be too specific (i.e. it will become outdated if we add another field in the future)

Maybe ChunkMetadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

if (idx < 0) {
idx = -idx-2; //points to element with offset closest to but less than 'start'
}
List<StoreKeyAndSizeAndOffset> ans = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use List.subList() here to avoid extra copies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, and another cool thing about using subList() is that I can do another binary search for the end index replacing the linearly scaling while loop and make this method a O(log(n)) operation (since subList() itself is a constant operation for ArrayList). Great suggestion!

List<Pair<StoreKey, Long>> orderedChunkIdList = indexToChunkIds.values().stream().collect(
Collectors.toList());

buf = MetadataContentSerDe.serializeMetadataContentV3(getBlobSize(), orderedChunkIdList);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to start writing in v3 before all frontends have been deployed with read support for metadata content v3.

Perhaps we should add a router config for the metadata content version like we have for blob id version and then pass the version number into the MetadataContentSerDe.serializeMetadataContent() method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added config

@dharju
Copy link
Contributor Author

dharju commented May 28, 2019

Will be adding more unit tests (specifically unit test for MessageFormatRecord) and incorporation into BlobIdTransformer

@dharju
Copy link
Contributor Author

dharju commented May 30, 2019

Fixing failing check, also changing PutManagerTest, GetManagerTest, and NonBlockingRouterTest to test against both old and new versions of metadata content

@dharju
Copy link
Contributor Author

dharju commented May 30, 2019

Looking at GetBlobOperationTest failure, appears to be triggered by random chunk size, indicates that a kind of corner case is missing from the test, will fix

@dharju
Copy link
Contributor Author

dharju commented May 30, 2019

Source of error was test with a very low probability of generating an incorrect test input. Fixing

/**
* The version to use for new metadata blobs.
*/
@Config("router.metadata.content.version")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to call this router.metadata.blob.content.version ?

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 like router.metadata.content.version because it matches the terminology used in the Metadata_Content_Format_* classes


/**
* Return the version of the metadata content record that this object was deserialized from
* @return
Copy link
Contributor

Choose a reason for hiding this comment

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

please complete this java doc (move line 140 to line 141)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

private CompositeBlobInfo deserializeMetadataContentV3(ByteBuffer metadataContent, StoreKeyFactory storeKeyFactory)
throws MessageFormatException, IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

no MessageFormatException thrown from this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

List<Pair<StoreKey, Long>> keyAndContentSizes = new ArrayList<>();
keyAndContentSizes.add(new Pair<>(keys.get(0), (long) chunkSizes[n][0]));
keyAndContentSizes.add(new Pair<>(keys.get(1), (long) chunkSizes[n][1]));
MetadataContentSerDe.serializeMetadataContentV3(totalSizes[n], keyAndContentSizes);
fail("Should have failed to serialize");
Copy link
Contributor

Choose a reason for hiding this comment

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

You may also test that keys with different size would throw exception here.

TransformationOutput output = transformer.transform(inputAndExpected.getInput());
assertNull("output exception should be null", output.getException());
verifyOutput(output.getMsg(), inputAndExpected.getExpected());
for (int i = 0; i < 2; i++) {
Copy link
Contributor

@jsjtzyy jsjtzyy Jun 4, 2019

Choose a reason for hiding this comment

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

How about moving this piece of code to BlobIdTransformerTest ctor and pass in parameters to test both V2 and V3 metadataContent in this class ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, changed

if (endIdx < 0) {
endIdx = -endIdx - 2; //points to element with offset closest to but less than 'end'
}
endIdx++;//List.subList expects an exclusive index
Copy link
Contributor

Choose a reason for hiding this comment

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

add a space before comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -62,4 +126,49 @@ public int getChunkSize() {
public List<StoreKey> getKeys() {
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative to materializing a list of StoreKeys for every CompositeBlobInfo is to return a "view" of chunkMetadataList:

  public List<StoreKey> getKeys() {
    return new AbstractList<StoreKey>() {
      @Override
      public StoreKey get(int index) {
        return chunkMetadataList.get(index).getStoreKey();
      }

      @Override
      public int size() {
        return chunkMetadataList.size();
      }
    };
  }

It seems like this would suffice for the two use cases that need the getKeys() method, the getChunkIdsOnly option in the router and BlobIdTransformer, as both of them are just interested in iterating through all of the keys and not mutating the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool idea, I'll use it

* - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
* | | | | | | | | |
* | version | total size | # of keys | size of | key1 | size of | key2 | ...... |
* |(2 bytes)| (8 bytes) | (4 bytes) | key1 blob | | key2 blob | | ...... |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the totalSize field meant for sanity checking that sum(chunksizes) == totalsize or are there other places where it is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's only used for checking

* @return the list of {@link ChunkMetadata} that lie upon the inclusive range
* between 'start' and 'end'
*/
public List<ChunkMetadata> getStoreKeysInByteRange(long start, long end) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to add unit tests for CompositeBlobInfo testing for edge cases of this method.
e.g.:

  • 0 byte chunks in the middle of the blob (Potentially a possibility with segmented blobs)
  • start/end = last byte of blob

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, actually writing that now. CompositeBlobInfo didn't have a unit test before because it was a POJO class but with this method it should be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain "0 byte chunks in the middle of the blob"?

Copy link
Contributor

@cgtz cgtz Jun 5, 2019

Choose a reason for hiding this comment

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

Sure. So I was thinking that since we allow 0 byte simple blob uploads, they are valid candidates for stitching. So, someone could potentially create a blob with chunk sizes like: 25, 30, 0, 0, 70, 0, 5

If we call this method with a start offset of 30, would the range start from the second or third chunk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in person, going to change the code so that stitched blobs will not allow blobs of 0 length to be stitched (will throw an exception if that happens)

*/
void initialize(int index, BlobId id) {
void initialize(int index, CompositeBlobInfo.ChunkMetadata keyAndSizeAndOffset) {
Copy link
Contributor

Choose a reason for hiding this comment

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

change variable names in this class (and others) from keyAndSizeAndOffset to chunkMetadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -937,9 +943,9 @@ protected ByteBuffer filterChunkToRange(ByteBuffer buf) {
buf.position(0);
buf.limit(0);
} else {
long startOffsetInThisChunk = chunkIndex == 0 ? resolvedByteRange.getStartOffset() % chunkSize : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about the confusion around chunkIndex. Since we now have the offset variable, you could consider changing this to long startOffsetInThisChunk = Math.max(resolvedByteRange.getStartOffset() - offset, 0) if you prefer.

long endOffsetInThisChunk =
chunkIndex == (numChunksTotal - 1) ? (resolvedByteRange.getEndOffset() % chunkSize) + 1 : chunkSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you feel that it is cleaner, you could also express this as long endOffsetInThisChunkExclusive = Math.min(resolvedByteRange.getEndOffset() - offset + 1, chunkSize)

@dharju
Copy link
Contributor Author

dharju commented Jun 5, 2019

made the max, min changes suggested above

chunkIdIterator = keys.listIterator();
numChunksTotal = keys.size();
dataChunks = new GetChunk[Math.min(keys.size(), NonBlockingRouter.MAX_IN_MEM_CHUNKS)];
chunkIdIterator = keysAndSizesAndOffsets.listIterator();
Copy link
Contributor

Choose a reason for hiding this comment

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

change this name to chunkMetadataList

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought I caught them all... done

@@ -943,8 +943,9 @@ protected ByteBuffer filterChunkToRange(ByteBuffer buf) {
buf.position(0);
buf.limit(0);
} else {
long startOffsetInThisChunk = Math.max(resolvedByteRange.getStartOffset() - offset, 0);
long endOffsetInThisChunkExclusive = Math.min(resolvedByteRange.getEndOffset() - offset + 1, chunkSize);
long startOffsetInThisChunk = chunkIndex == 0 ? resolvedByteRange.getStartOffset() - offset : 0;
Copy link
Contributor

@cgtz cgtz Jun 7, 2019

Choose a reason for hiding this comment

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

Why did you decide to revert the min/max change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It broke a GetBlobOperationTest test so I changed it back

@@ -1051,7 +1057,6 @@ protected void maybeProcessCallbacks() {
decryptCallbackResultInfo.result.getDecryptedUserMetadata().array());
}
totalSize = decryptCallbackResultInfo.result.getDecryptedBlobContent().remaining();
chunkSize = totalSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you choose to use min/max in filterChunkToRange, I think it will be required to set chunkSize appropriately on simple blobs

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.

One last comment, but other than that LGTM

@@ -1604,12 +1612,21 @@ private void finalizeMetadataChunk() {
passedInBlobProperties.isEncrypted(), passedInBlobProperties.getExternalAssetTag());
if (isStitchOperation() || getNumDataChunks() > 1) {
// values returned are in the right order as TreeMap returns them in key-order.
List<StoreKey> orderedChunkIdList = new ArrayList<>(indexToChunkIds.values());
buf = MetadataContentSerDe.serializeMetadataContent(intermediateChunkSize, getBlobSize(), orderedChunkIdList);
List<Pair<StoreKey, Long>> orderedChunkIdList = new ArrayList<>(indexToChunkIdsAndChunkSizes.values());
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to inside the else if (line 1620) so that we don't have to construct it for v2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cgtz cgtz merged commit 580097e into linkedin:master Jun 10, 2019
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.

4 participants