Hi Sudheer,
I reviewed TS-2314 and had a few comments ...
The records.config.en.rst documentation is not as clear as it could be. From
our IRC discussion, it seems like the values should be:
0 - never read while writing
1 - always read while writing
2 - always read while writing, but allow non-cached Range requests
through to the origin
HttpSM::parse_range_done should not be needed. As you showed me, I can see how
this is called multiple times via HttpSM::do_range_setup_if_necessary(), but
that is the right place to short-circuit this (due to the ..if_necessary()
naming of this method).
HttpSM::parse_range_and_compare() should not be checking the configuration
values; it's too low level. When you parse the range header here, just keep
track of whether the ranges are in cache.
In HttpSM::parse_range_and_compare(), get_frag_offset_count() can return zero.
Can it ever return zero at this point in the code? If so, you'll index -1 into
the frag_offset_tbl array.
HttpSM::do_range_setup_if_necessary is where the configuration policy check
should take place. The
I'm not sure that the early exit on the "if (!t_state.range_in_cache)"
condition is correct. If there are multiple ranges and they are not all in
cache, the transform tunnel won't be set up. Is that desirable?
The second check in HttpSM::do_range_setup_if_necessary(),
"cache_sm.cache_read_vc->is_pread_capable() || t_state.range_in_cache", will be
true if is_pread_capable is false, which seems to contradict the original code.
This is an example of spooky action at a distance, since t_state.range_in_cache
has already checked a different is_pread_capable condition, so it's really hard
to know what the right conditions are here.
cheers,
James
