On Sep 24, 2014, at 10:03 AM, James Peach <jpe...@apache.org> wrote: > 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.
Also, cache_config_read_while_writer is not used in HttpTransact.cc. I'm not sure that the "(s->range_setup == RANGE_NOT_HANDLED) || !s->range_in_cache" condition is safe. Maybe this implies that range_in_cache could/should be subsumed by additional range_setup values? J
