Adds test cases for readVectored()

[ahmarsuhail]

Description of change

Adds in more test cases for readVectored()

Relevant issues

Does this contribution introduce any breaking changes to the existing APIs or behaviors?

Does this contribution introduce any new public APIs or behaviors?

How was the contribution tested?

Does this contribution need a changelog entry?

• I have updated the CHANGELOG or README if appropriate

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

[SanjayMarreddi]

Thanks a lot for the PR!

I just want to see your thoughts on if we can move some of these tests to unit-tests as they test a single logical unit of functionality ( like validating the ranges )

[SanjayMarreddi]

Should we make testNullRangeList a unit test at S3SeekableInputStream level as it just checks the arguments at the same level?

[ahmarsuhail]

done, thanks

[rajdchak]

Not related to this change, but can we make sure these tests are not using the default seekable stream configurations as they can cause out of memory errors, can you check read correctness or concurrency correctness tests I had addressed this issue in those.

[ahmarsuhail]

done, thanks!

[fuatbasik]

It might be good to consider certain test cases that will test known underlying interactions here. For example, test cases where ranges will span multiple blocks or where multiple ranges are accessing to same block.

[fuatbasik]

I feel like we really need to think critically about test cases for read vectored including example i gave before. We should consider different failure modes in detail and make sure that we are protected against them.

[fuatbasik]

Thanks @ahmarsuhail please see my comments inline.

[SanjayMarreddi]

May be a nit, but I think this logic can be simplified if we update our ReadMode enum class to something like below: ( The boolean values assigned may be wrong, but syntax should be like that ). Then in this blockManager, we could just use readMode.allowsSequentialPrefetch() value and remove the above method. In the future, if we have more readModes, we dont have to make changes in both enum class and this method.

public enum ReadMode {
  SYNC(true),
  ASYNC(true),
  SMALL_OBJECT_PREFETCH(true),
  SEQUENTIAL_FILE_PREFETCH(true),
  READ_VECTORED(false),
  REMAINING_COLUMN_PREFETCH(false),
  COLUMN_PREFETCH(false),
  DICTIONARY_PREFETCH(false),
  PREFETCH_TAIL(false),

  private final boolean allowsSequentialPrefetch;

  ReadMode(boolean allowsSequentialPrefetch) {
    this.allowsSequentialPrefetch = allowsSequentialPrefetch;
  }

  public boolean allowsSequentialPrefetch() {
    return allowsSequentialPrefetch;
  }
}

[ahmarsuhail]

thanks this is a good suggestion :) will do

[fuatbasik]

Can a single IOPlan have multiple ranges with different readmodes? If yes, this would not handle that case. If now, should we make ReadMode a property of IOPlan?

[ahmarsuhail]

no, single IoPlan should not have multiple readModes. so this is ok here

[fuatbasik]

thanks a lot @ahmarsuhail . I put 2-3 comments. I have a question about concurrency in one of my comments if you think that is OK my concern is not valid, i am happy to approve

[fuatbasik]

I am concerned about concurrency here. The scenario I have in mind 2 streams against the same S3 key calling readvectored concurrently with overlapping ranges. Would that be a problem?

[fuatbasik]

@ahmarsuhail thanks a lot! LGTM.