Issue 5458: (SLTS) For read operation, read all chunks in parallel.

[sachin-j-joshi]

Signed-off-by: Sachin Joshi sachin.joshi@emc.com

Change log description
ReadOperation reads all the relevant chunks in parallel when multiple chunks need to be read in a single call.

Purpose of the change
Fixes #5458

What the code does
ReadOperation reads all the relevant chunks in parallel when multiple chunks need to be read in a single call.

How to verify it
Existing tests should cover this scenario already.
All existing tests should pass.

[codecov]

Codecov Report

Merging #5459 (fa4cec3) into master (ddda465) will increase coverage by 0.48%.
The diff coverage is 97.36%.

@@             Coverage Diff              @@
##             master    #5459      +/-   ##
============================================
+ Coverage     84.26%   84.74%   +0.48%     
- Complexity     8970    13869    +4899     
============================================
  Files           634      917     +283     
  Lines         32781    51355   +18574     
  Branches       2982     5299    +2317     
============================================
+ Hits          27623    43521   +15898     
- Misses         3483     4943    +1460     
- Partials       1675     2891    +1216     

Impacted Files	Coverage Δ	Complexity Δ
...segmentstore/storage/chunklayer/ReadOperation.java	90.47% <97.36%> (ø)	40.00 <9.00> (?)
...ient/segment/impl/AsyncSegmentInputStreamImpl.java	84.02% <0.00%> (-2.09%)	28.00% <0.00%> (-1.00%)
...ga/controller/task/Stream/StreamMetadataTasks.java	84.24% <0.00%> (-0.37%)	343.00% <0.00%> (-1.00%)
...mentstore/server/host/stat/AutoScaleProcessor.java	83.20% <0.00%> (ø)	44.00% <0.00%> (?%)
...a/io/pravega/segmentstore/storage/SyncStorage.java	100.00% <0.00%> (ø)	4.00% <0.00%> (?%)
...gmentstore/storage/StorageNotPrimaryException.java	62.50% <0.00%> (ø)	3.00% <0.00%> (?%)
.../server/writer/ReconciliationFailureException.java	87.50% <0.00%> (ø)	2.00% <0.00%> (?%)
.../segmentstore/server/tables/ContainerKeyIndex.java	91.48% <0.00%> (ø)	72.00% <0.00%> (?%)
...server/logs/operations/DeleteSegmentOperation.java	88.88% <0.00%> (ø)	5.00% <0.00%> (?%)
... and 280 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 ddda465...fa4cec3. Read the comment docs.

[RaulGracia]

While I understand the potential benefits of the PR, I have some questions/concerns:

• How do we ensure that blindly issuing parallel reads to storage is not going to have side effects, like overwhelming the cache?
• How do we guarantee that this approach is not going to induce importante interferences in the operation of StorageWriter if many parallel reads are getting most threads of the storage pool?
• We need tests to validate the above scenarios in some way, and also that the change in updating the atomic counters (e.g., currentBufferOffset) is right, even if one of the storage reads fails.

[sachin-j-joshi]

To summarize the change

• First of all the upper layers are issuing single read request to read from given offset data of size length and for that they have already allocated and provided a byte array. That remains as it is. This change does not affect anything related to that.
• With SLTS no append mode, it is more likely that a given range may contain more than one chunk to be read to satisfy the request.
• All we are doing here is that, instead of reading and filling up that byte array sequentially by reading chunks one by one, we are now issuing parallel reads for individual chunks and filling up sections of that array in parallel.
• The various counters are basically used only to calculate what chunks to read and at what offsets in buffer data gets copied etc. None of that calculation changes because of parallelizing the actual reads.
• In terms of failures, for Storage.read() request to succeed all chunk reads must succeed. That requirement has not changed.
• This change should have no effect on cache. It was the cache that issued that single read request on segment we are just parallelizing inside the execution of that same request.
• We do want reads to have higher priority and lower latency than writes. Reads are often issued as a direct result of client request. Writes can wait, reads shouldn't.
• At this point I do not think reads can starve writes. (We'll have to fix any issues in case we actually see starving in perf/stress tests.)

Testing

• Almost each and every SLTS unit test reads data, so this code is exercised by existing tests. (and that coverage can be definitely improved. )

[sachin-j-joshi]

Added some additional tests.

Coverage looks better now