Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2548343002:
NTPSnippets: Set MaxRetriesOn5xx only for interactive requests. (Closed)
DescriptionSet MaxRetriesOn5xx only for interactive requests.
Keep retries on 5xx errors only for user-triggered, interactive requests.
Other fetches have no immediate priority and their number will
be determined by a variation parameter.
This CL does not reschedule/retry non-interactive requests,
as we figured that this small change improves the current state on its own and rescheduling requires some more discussion.
BUG=668074
Committed: https://crrev.com/6d88a9de178e49cfd9f2660d3ba1d7a1069c18ff
Cr-Commit-Position: refs/heads/master@{#437872}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use TestURLFetcher instead of helper function. Rebased. #
Total comments: 18
Patch Set 3 : Setting retries configurable by VariationParameter. #Patch Set 4 : Replaced inherited test with scope-injected fetcher. #
Total comments: 31
Patch Set 5 : Attached the variation parameter to the suggestions feature. #Patch Set 6 : Ability of FetcherFactory to warn about multiple |Create| calls. #
Total comments: 17
Patch Set 7 : Rebased. #
Total comments: 8
Patch Set 8 : Desriptions for test cases. #Patch Set 9 : Document missing functionality inline. #
Messages
Total messages: 45 (22 generated)
Description was changed from ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority and will be postponed to an hour later (plus jitter of an hour). BUG=668074 ========== to ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority. BUG=668074 ==========
Description was changed from ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority. BUG=668074 ========== to ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority. This CL does not reschedule/retry non-interactive requests, as we figured that this small change improves the current state on its own. BUG=668074 ==========
fhorschig@chromium.org changed reviewers: + treib@chromium.org
This is the CL I spoke about. It would be nice if you would also take an OWNER look. I can also ask someone else this time (like jkrcal@ ;D).
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2548343002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2548343002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:666: return BuildURLFetcher(/*request=*/nullptr, // Can never be called anyways. this is a smell. If you really need something like this, then subclass the builder, do this modification there and test it. But again, I'd much rather have the test using the public interface and test the actual class. https://codereview.chromium.org/2548343002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2548343002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.h:228: std::string PreviewRequestBodyForTesting() const { return BuildBody(); } hmm... this has a smell and doesn't seem to scale well. Usually, a test should use the public interface and observe the expected behavior or output. In this case it's tricky as the built request object wont' have the necessary information. What you want to verify is an interaction with the external world (through the URL fetcher). Can't we inject a fake or mock fetcher in the test? One like this https://codesearch.chromium.org/chromium/src/net/url_request/test_url_fetcher... via https://codesearch.chromium.org/chromium/src/net/url_request/url_fetcher_impl... ?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The preview functions are deleted. This is now the most basic change. https://codereview.chromium.org/2548343002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2548343002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:666: return BuildURLFetcher(/*request=*/nullptr, // Can never be called anyways. On 2016/12/05 17:26:33, tschumann wrote: > this is a smell. If you really need something like this, then subclass the > builder, do this modification there and test it. But again, I'd much rather have > the test using the public interface and test the actual class. Removed. As noted in your last comment, this was easy to do with the TestURLFetcher. https://codereview.chromium.org/2548343002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2548343002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.h:228: std::string PreviewRequestBodyForTesting() const { return BuildBody(); } On 2016/12/05 17:26:33, tschumann wrote: > hmm... this has a smell and doesn't seem to scale well. > > Usually, a test should use the public interface and observe the expected > behavior or output. > > In this case it's tricky as the built request object wont' have the necessary > information. What you want to verify is an interaction with the external world > (through the URL fetcher). > > Can't we inject a fake or mock fetcher in the test? One like this > https://codesearch.chromium.org/chromium/src/net/url_request/test_url_fetcher... > via > https://codesearch.chromium.org/chromium/src/net/url_request/url_fetcher_impl... > ? Reverted changes made here and added a TODO. In order to inject the URLFetcher for a specific builder, we would need to build and start the request or expose the request. Both alternatives don't improve the code in any way. As soon as the Builder and JsonRequest have moved to snippets::internal, these functions are completely unnecessary.
https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:820: // TODO(fhorschig): back off for 1h (+1h jitter) and then retry. I'm not convinced we actually want to do this :) Maybe better put this discussion in the bug? https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:821: url_fetcher->SetMaxRetriesOn5xx(0); As discussed offline: Should we introduce a variation parameter to control this? https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:364: net::TestURLFetcher* fetcher = test_url_fetcher_factory_.GetFetcherByID(0); optional, to make this slightly less magic: It's possible to specify an ID when creating the URLFetcher (in NTPSnippetsFetcher). Or maybe just add a comment why ID 0 is all we need here? https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:365: return fetcher; nit: just return test_url_fetcher_factory_.GetFetcherByID(0); https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:384: ASSERT_THAT(fetcher->delegate(), NotNull()); Also assert that |fetcher| itself isn't null? https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:387: net::URLRequestStatus(net::URLRequestStatus::FAILED, -2)); nit: What's -2? Maybe add a /*name=*/? https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:991: // Fetch interactive and expect that the created fechter would retry on 5xx. nit: s/fechter/fetcher/ https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:996: CloseLastUrlFetcher(); nit: Split this into two tests, so that this explicit call isn't needed?
some thoughts about the test design https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:352: class NTPSnippetsInternalUrlFetcherTest : public NTPSnippetsFetcherTest { sorry for the bystander, but it seems like inheritance is the wrong tool here and aggregation would be better suited. Why not make this a helper class which can be instantiated within the TEST() blocks? This would make all the life-time issues go away. Something like: // ScopedInjectedURLFetcher injects url fetcher instances and executes the fetcher with hard-coded responses on deletion. class ScopedInjectedURLFetcher { public: ScopedInjectedURLFetcher() {} ~ScopedInjectedURLFetcher() { net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher(); if (fetcher) { // inline CloseLastUrlFetcher() here. You can actually also just iterate over all created fetchers and close all calls -- shouldn't make a difference. ... } } NTPSnippetsFetcher::SnippetsAvailableCallback CreateSnippetsAvailableCallback( MockSnippetsAvailableCallback* callback) { // Make sure the callback gets executed. EXPECT_CALL(...); ... } private: net::TestURLFetcherFactory test_url_fetcher_factory_; }; In your TEST() you can then simply instantiate an instance and if you want to verify the effects after the fetcher returned, you can simply put them in a block, like: TEST_F(...) { { ScopedInjectedURLFetcher url_fetcher; // set up call, do something. } // verify state after the execution.
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the comments. Variations added and Inherited Test class removed. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:820: // TODO(fhorschig): back off for 1h (+1h jitter) and then retry. On 2016/12/06 16:06:56, Marc Treib wrote: > I'm not convinced we actually want to do this :) > Maybe better put this discussion in the bug? Done already. I removed this comment for now as I won't submit this CL anyway until I know if it is actually wanted behavior. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:821: url_fetcher->SetMaxRetriesOn5xx(0); On 2016/12/06 16:06:56, Marc Treib wrote: > As discussed offline: Should we introduce a variation parameter to control this? Done. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:352: class NTPSnippetsInternalUrlFetcherTest : public NTPSnippetsFetcherTest { On 2016/12/06 18:18:25, tschumann wrote: > sorry for the bystander, but it seems like inheritance is the wrong tool here > and aggregation would be better suited. > > Why not make this a helper class which can be instantiated within the TEST() > blocks? This would make all the life-time issues go away. > Something like: > > // ScopedInjectedURLFetcher injects url fetcher instances and executes the > fetcher with hard-coded responses on deletion. > class ScopedInjectedURLFetcher { > public: > ScopedInjectedURLFetcher() {} > ~ScopedInjectedURLFetcher() { > net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher(); > if (fetcher) { > // inline CloseLastUrlFetcher() here. You can actually also just iterate > over all created fetchers and close all calls -- shouldn't make a difference. > ... > } > } > NTPSnippetsFetcher::SnippetsAvailableCallback > CreateSnippetsAvailableCallback( > MockSnippetsAvailableCallback* callback) { > // Make sure the callback gets executed. > EXPECT_CALL(...); > ... > } > > private: > net::TestURLFetcherFactory test_url_fetcher_factory_; > }; > > > In your TEST() you can then simply instantiate an instance and if you want to > verify the effects after the fetcher returned, you can simply put them in a > block, like: > TEST_F(...) { > { > ScopedInjectedURLFetcher url_fetcher; > // set up call, do something. > } > // verify state after the execution. Thank you for the bystander comment! I was already looking for a better alternative (to be consistent with CL 2552813005) and this idea works great and with much less explanation. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:364: net::TestURLFetcher* fetcher = test_url_fetcher_factory_.GetFetcherByID(0); On 2016/12/06 16:06:56, Marc Treib wrote: > optional, to make this slightly less magic: It's possible to specify an ID when > creating the URLFetcher (in NTPSnippetsFetcher). Or maybe just add a comment why > ID 0 is all we need here? Done. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:365: return fetcher; On 2016/12/06 16:06:56, Marc Treib wrote: > nit: just > return test_url_fetcher_factory_.GetFetcherByID(0); Done. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:384: ASSERT_THAT(fetcher->delegate(), NotNull()); On 2016/12/06 16:06:56, Marc Treib wrote: > Also assert that |fetcher| itself isn't null? Done. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:387: net::URLRequestStatus(net::URLRequestStatus::FAILED, -2)); On 2016/12/06 16:06:56, Marc Treib wrote: > nit: What's -2? Maybe add a /*name=*/? Done. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:991: // Fetch interactive and expect that the created fechter would retry on 5xx. On 2016/12/06 16:06:56, Marc Treib wrote: > nit: s/fechter/fetcher/ Done. https://codereview.chromium.org/2548343002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:996: CloseLastUrlFetcher(); On 2016/12/06 16:06:56, Marc Treib wrote: > nit: Split this into two tests, so that this explicit call isn't needed? Done.
https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:65: // Variation parameter for disabling the retry . nitty nit: no space before "." https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:66: const char kNumberOf5xxRetries[] = "number_of_non_interactive_5xx_retries"; nit1: Rename the variable to make clear it's a param name, not an actual value. (Maybe just append "Name" to the name :) ) nit2: "background_5xx_retry_count"? (our other params use "_count" rather than "number_of_", and "background" is IMO more descriptive than "non_interactive") https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:142: return retries.empty() ? 0 : std::max(0, std::stoi(retries)); Use GetParamAsInt from ntp_snippets/features.h. It should also be attached to ntp_snippets::kArticleSuggestionsFeature rather than to the study name. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:829: // Don't retry immediately if a scheduled fetch fails. Comment is now not accurate anymore. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:830: url_fetcher->SetMaxRetriesOn5xx(GetNumberOfNonInteractiveRetries()); Make a "GetNumberOf5xxRetries(bool interactive)" helper? Then SetMaxRetriesOn5xx can be called only once. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:232: // which index was used. nit: some extra newlines here https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:234: // manuall first (by calling |CloseLastUrlFetcher|) in order to secure s/manuall/manually/ https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:290: return ToSnippetsAvailableCallback(&mock_callback()); Hm, how about inlining this into the tests themselves? I think it's nicer to have the expectations in the test directly. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:999: std::map<std::string, int> expected_retries_for_configurations = { Make this const? I'd also slightly prefer an array of structs, since you don't actually need the map functionality. Also the values will have better names thant "first" and "second" :) It can be initialized in almost exactly the same way. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:1005: for (auto retry_configuration : expected_retries_for_configurations) { const auto& ?
Comment on the description (per the recent discussion about that): Would you mind adding "NTPSnippets: " or something similar to the title (to make life easier for sheriffs or other people who have to read the git logs)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority. This CL does not reschedule/retry non-interactive requests, as we figured that this small change improves the current state on its own. BUG=668074 ========== to ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority and their number will be determined by a variation parameter. This CL does not reschedule/retry non-interactive requests, as we figured that this small change improves the current state on its own and rescheduling requires some more discussion. BUG=668074 ==========
cool https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:212: class ScopedInjectedTestURLFetcherFactory { now that I better understand the usage pattern, maybe we can find a shorter name. ScopedTestURLFetcherFactory? Also please: -- move it out of the test class's scope. There's no need for that coupling. -- add a class comment explaining it's use case, e.g.: // <class-name> can be used to temporarily inject fake url fetcher instances. Client code can access created fetchers to verify expected properties. When the factory gets destroyed, all initiated fetches will be completed. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:216: net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher(); either clearly document that we only support creating a single fetcher (in which case GetLastCreatedFetcher should check that there's only a single one existing), or support multiple fetchers at destruction -- just iterating over all of them (no need to provide more accessors though as no test needs them). https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:290: return ToSnippetsAvailableCallback(&mock_callback()); On 2016/12/08 10:52:06, Marc Treib wrote: > Hm, how about inlining this into the tests themselves? I think it's nicer to > have the expectations in the test directly. +1 https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:999: std::map<std::string, int> expected_retries_for_configurations = { On 2016/12/08 10:52:06, Marc Treib wrote: > Make this const? > I'd also slightly prefer an array of structs, since you don't actually need the > map functionality. Also the values will have better names thant "first" and > "second" :) > It can be initialized in almost exactly the same way. Using a struct would be better indeed -- you seem to abusing the keys here (see the example of the empty string key). I'd vote for a vector<> instead of an array ;-)
https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:999: std::map<std::string, int> expected_retries_for_configurations = { On 2016/12/08 13:27:53, tschumann wrote: > On 2016/12/08 10:52:06, Marc Treib wrote: > > Make this const? > > I'd also slightly prefer an array of structs, since you don't actually need > the > > map functionality. Also the values will have better names thant "first" and > > "second" :) > > It can be initialized in almost exactly the same way. > Using a struct would be better indeed -- you seem to abusing the keys here (see > the example of the empty string key). > I'd vote for a vector<> instead of an array ;-) Right, vector also works, now that we have std::initializer_list. Doesn't really make a difference IMO, since it'll only be used in a single range-based for loop below, which works the same in either case.
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
Debugging took me some time, but now, the factory will work even if someone changes this "all ids are 0" thing. Tests are more robust and the factory can easily be reused and will not break but warn and act if it's scope it too wide for a single fetcher. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:65: // Variation parameter for disabling the retry . On 2016/12/08 10:52:06, Marc Treib wrote: > nitty nit: no space before "." Oh no, a Plenk! (https://en.wikipedia.org/wiki/Plenken) https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:66: const char kNumberOf5xxRetries[] = "number_of_non_interactive_5xx_retries"; On 2016/12/08 10:52:06, Marc Treib wrote: > nit1: Rename the variable to make clear it's a param name, not an actual value. > (Maybe just append "Name" to the name :) ) > > nit2: "background_5xx_retry_count"? (our other params use "_count" rather than > "number_of_", and "background" is IMO more descriptive than "non_interactive") Done. Done. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:142: return retries.empty() ? 0 : std::max(0, std::stoi(retries)); On 2016/12/08 10:52:06, Marc Treib wrote: > Use GetParamAsInt from ntp_snippets/features.h. > It should also be attached to ntp_snippets::kArticleSuggestionsFeature rather > than to the study name. Done. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:829: // Don't retry immediately if a scheduled fetch fails. On 2016/12/08 10:52:06, Marc Treib wrote: > Comment is now not accurate anymore. Done. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:830: url_fetcher->SetMaxRetriesOn5xx(GetNumberOfNonInteractiveRetries()); On 2016/12/08 10:52:06, Marc Treib wrote: > Make a "GetNumberOf5xxRetries(bool interactive)" helper? Then SetMaxRetriesOn5xx > can be called only once. Done. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:212: class ScopedInjectedTestURLFetcherFactory { On 2016/12/08 13:27:53, tschumann wrote: > now that I better understand the usage pattern, maybe we can find a shorter > name. > ScopedTestURLFetcherFactory? > > Also please: > -- move it out of the test class's scope. There's no need for that coupling. > -- add a class comment explaining it's use case, e.g.: > // <class-name> can be used to temporarily inject fake url fetcher instances. > Client code can access created fetchers to verify expected properties. When the > factory gets destroyed, all initiated fetches will be completed. Moved. Commented. It is now a Factory itself and implements the "ScopedURLFetcherFactory" interface. Therefore, we can focus on what it actually does: Ensuring the callback is called. This is most important and everything else can be done by the existing class. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:216: net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher(); On 2016/12/08 13:27:53, tschumann wrote: > either clearly document that we only support creating a single fetcher (in which > case GetLastCreatedFetcher should check that there's only a single one > existing), or support multiple fetchers at destruction -- just iterating over > all of them (no need to provide more accessors though as no test needs them). We support now creation of multiple fetchers [1], but access to the last. |GetLastCreatedFetcher| cannot check for multiple instances [2]. Therefore this class _is_ now a factory (which makes sense) that keeps track of created fetchers which still need their delegates called. [1] There is a delegate for testing purposes which allows calling the delegate during destruction of the fetcher. [2] No access to the map of fetchers and even if: by default new fetchers receive the ID 0. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:232: // which index was used. On 2016/12/08 10:52:06, Marc Treib wrote: > nit: some extra newlines here Done. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:234: // manuall first (by calling |CloseLastUrlFetcher|) in order to secure On 2016/12/08 10:52:06, Marc Treib wrote: > s/manuall/manually/ Done. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:290: return ToSnippetsAvailableCallback(&mock_callback()); On 2016/12/08 10:52:06, Marc Treib wrote: > Hm, how about inlining this into the tests themselves? I think it's nicer to > have the expectations in the test directly. Removed. As they cannot happen anymore with the delegate-calling factory. This expectation was pretty much independent from the test itself and just serves to catches memory leaks without usage of local ASan builds. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:999: std::map<std::string, int> expected_retries_for_configurations = { On 2016/12/08 10:52:06, Marc Treib wrote: > Make this const? > I'd also slightly prefer an array of structs, since you don't actually need the > map functionality. Also the values will have better names thant "first" and > "second" :) > It can be initialized in almost exactly the same way. Done. https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:1005: for (auto retry_configuration : expected_retries_for_configurations) { On 2016/12/08 10:52:06, Marc Treib wrote: > const auto& > ? Done.
https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:170: class DelegateCallingTestURLFetcherFactory hmm... I wonder why the previous (much simpler version) didn't work. Was there anything missing in particular? Might be nice to add a comment why we need to inherit from the factory instead of just using it. (did I mention that inheritance is something we should only do if we have to? ;-)). https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:205: return GetFetcherByID(fetcher_id_queue_.front()); shouldn't this be back? https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:209: std::list<int> fetcher_id_queue_; // Use a list as searching is essential. drop the type in the variable name (it's outdated already ;-)). Just call it fetchers_.
https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:65: // Variation parameter for disabling the retry . On 2016/12/09 12:37:54, fhorschig wrote: > On 2016/12/08 10:52:06, Marc Treib wrote: > > nitty nit: no space before "." > > Oh no, a Plenk! > (https://en.wikipedia.org/wiki/Plenken) Haha, I didn't know that term! I have to admit though that this kind of thing bothers me way more than it should. So know I'll know what to call it :D https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:142: return retries.empty() ? 0 : std::max(0, std::stoi(retries)); On 2016/12/09 12:37:54, fhorschig wrote: > On 2016/12/08 10:52:06, Marc Treib wrote: > > Use GetParamAsInt from ntp_snippets/features.h. > > It should also be attached to ntp_snippets::kArticleSuggestionsFeature rather > > than to the study name. > > Done. Heads-up: As of https://codereview.chromium.org/2558743003/, the helpers have moved into components/variations. https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:164: // url fetcher instances into a scope. s/fake url fetcher/TestURLFetcher/ ? Since both TestURLFetcher and FakeURLFetcher exist, we shouldn't mix the terms. (I can never remember which is which anyway...) https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:191: LOG(ERROR) << "The ID " << id << " was already assigned to a fetcher." Is this actually an error? If so, should we just fail the test? https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:209: std::list<int> fetcher_id_queue_; // Use a list as searching is essential. You mean, this can't be std::queue because that doesn't support iterating? Then I'd say that, because the message is really "don't use queue" more than "use list". std::deque would also work btw :) nit: data members usually come after methods https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:224: if (!fetcher->delegate()) { nit: Could merge the two ifs: if (!fetcher || !fetcher->delegate()) https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:375: void SetVariationParametersForFeatures( Hm, now SetFetchingPersonalizationVariation and SetVariationParametersForFeatures conflict (as in, you can't call both). Could we get rid of SetFetchingPersonalizationVariation? Looks like it provides a subset of SetVariationParametersForFeatures's functionality.
https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:65: // Variation parameter for disabling the retry . On 2016/12/09 12:59:24, Marc Treib wrote: > On 2016/12/09 12:37:54, fhorschig wrote: > > On 2016/12/08 10:52:06, Marc Treib wrote: > > > nitty nit: no space before "." > > > > Oh no, a Plenk! > > (https://en.wikipedia.org/wiki/Plenken) > > Haha, I didn't know that term! > I have to admit though that this kind of thing bothers me way more than it > should. So know I'll know what to call it :D You are not alone ;) https://codereview.chromium.org/2548343002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:142: return retries.empty() ? 0 : std::max(0, std::stoi(retries)); On 2016/12/09 12:59:24, Marc Treib wrote: > On 2016/12/09 12:37:54, fhorschig wrote: > > On 2016/12/08 10:52:06, Marc Treib wrote: > > > Use GetParamAsInt from ntp_snippets/features.h. > > > It should also be attached to ntp_snippets::kArticleSuggestionsFeature > rather > > > than to the study name. > > > > Done. > > Heads-up: As of https://codereview.chromium.org/2558743003/, the helpers have > moved into components/variations. Done. https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:164: // url fetcher instances into a scope. On 2016/12/09 12:59:24, Marc Treib wrote: > s/fake url fetcher/TestURLFetcher/ ? > Since both TestURLFetcher and FakeURLFetcher exist, we shouldn't mix the terms. > (I can never remember which is which anyway...) Done. https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:170: class DelegateCallingTestURLFetcherFactory On 2016/12/09 12:53:38, tschumann wrote: > hmm... I wonder why the previous (much simpler version) didn't work. Was there > anything missing in particular? > Might be nice to add a comment why we need to inherit from the factory instead > of just using it. > (did I mention that inheritance is something we should only do if we have to? > ;-)). Yes, you mentioned that. I replaced that multiple times with composition but this is the best thing I ended up with. What I could do is inheriting from the base factory interface [1, 2] and make this class a facade that uses another factory internally [3]. But this way less readable and from a logic perspective: this class really IS a TestURLFetcher. [1] I need to control the point where fetchers are created. [2] I needed to implement other interfaces like ScopedURLFetcherFactory, too. ... just to reach the same functionality again. [3] It's doing the same thing and this would be a major code duplication otherwise. https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:191: LOG(ERROR) << "The ID " << id << " was already assigned to a fetcher." On 2016/12/09 12:59:24, Marc Treib wrote: > Is this actually an error? If so, should we just fail the test? It's a warning now. I thought about just failing but if you don't care about the timing of a callback that much or if it's empty, it's just an unnecessary restriction. Warning is probably more appropriate, because it might still work out. https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:205: return GetFetcherByID(fetcher_id_queue_.front()); On 2016/12/09 12:53:38, tschumann wrote: > shouldn't this be back? What came in first should be handled first (--> queue ;D). I used push_back for adding elements, so the newest is at front(). https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:209: std::list<int> fetcher_id_queue_; // Use a list as searching is essential. On 2016/12/09 12:53:38, tschumann wrote: > drop the type in the variable name (it's outdated already ;-)). Just call it > fetchers_. @tschumann: Done. @treib: std::deque it is. And done. https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:224: if (!fetcher->delegate()) { On 2016/12/09 12:59:24, Marc Treib wrote: > nit: Could merge the two ifs: > if (!fetcher || !fetcher->delegate()) Okay. https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:375: void SetVariationParametersForFeatures( On 2016/12/09 12:59:24, Marc Treib wrote: > Hm, now SetFetchingPersonalizationVariation and > SetVariationParametersForFeatures conflict (as in, you can't call both). Could > we get rid of SetFetchingPersonalizationVariation? Looks like it provides a > subset of SetVariationParametersForFeatures's functionality. The basic difference is that one is tied to a feature. But in CL 2552813005 |SetVariationParametersForFeatures| will disappear for the reason you mentioned.
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:170: class DelegateCallingTestURLFetcherFactory On 2016/12/12 09:54:03, fhorschig wrote: > On 2016/12/09 12:53:38, tschumann wrote: > > hmm... I wonder why the previous (much simpler version) didn't work. Was there > > anything missing in particular? > > Might be nice to add a comment why we need to inherit from the factory instead > > of just using it. > > (did I mention that inheritance is something we should only do if we have to? > > ;-)). > > Yes, you mentioned that. I replaced that multiple times with composition but > this is the best thing I ended up with. > > What I could do is inheriting from the base factory interface [1, 2] and make > this class a facade that uses another factory internally [3]. > But this way less readable and from a logic perspective: this class really IS a > TestURLFetcher. > > [1] I need to control the point where fetchers are created. > [2] I needed to implement other interfaces like ScopedURLFetcherFactory, too. > ... just to reach the same functionality again. > [3] It's doing the same thing and this would be a major code duplication > otherwise. to be more precise, what's the new bit of functionality that requires us from inheriting from TestURLFetcherFactory? My concern is that your code now has to deal with a lot of factory implementation details which we didn't have to do before (dealing with delegates, creating the fetchers). And in the end, i'm not sure it's truely a factory -- the creation is still forwarded to the factory we inherit from. And the test doesn't want to deal with a factory -- we just want to inject our fetchers somehow. I'm not saying we can't do it like you propose, but I'd like to understand why we need to deal with all the details (which have a good potential for bugs) ;-)
Thanks! LGTM (with nits) once Tim is satisfied. https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:222: return; When would |fetcher| be null here? Add a comment to explain? https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:288: struct ExpectationForVariationParam { nit: Move this into the test that uses it. https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:1053: {"", 0}, // Keep 2 retries if no variation is set. Comment is out of date. One alternative is adding a "description" string to each test case. Then when one of them fails, we can print it, so it's clear what exactly failed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:170: class DelegateCallingTestURLFetcherFactory On 2016/12/12 10:00:42, tschumann wrote: > On 2016/12/12 09:54:03, fhorschig wrote: > > On 2016/12/09 12:53:38, tschumann wrote: > > > hmm... I wonder why the previous (much simpler version) didn't work. Was > there > > > anything missing in particular? > > > Might be nice to add a comment why we need to inherit from the factory > instead > > > of just using it. > > > (did I mention that inheritance is something we should only do if we have > to? > > > ;-)). > > > > Yes, you mentioned that. I replaced that multiple times with composition but > > this is the best thing I ended up with. > > > > What I could do is inheriting from the base factory interface [1, 2] and make > > this class a facade that uses another factory internally [3]. > > But this way less readable and from a logic perspective: this class really IS > a > > TestURLFetcher. > > > > [1] I need to control the point where fetchers are created. > > [2] I needed to implement other interfaces like ScopedURLFetcherFactory, too. > > ... just to reach the same functionality again. > > [3] It's doing the same thing and this would be a major code duplication > > otherwise. > > to be more precise, what's the new bit of functionality that requires us from > inheriting from TestURLFetcherFactory? > My concern is that your code now has to deal with a lot of factory > implementation details which we didn't have to do before (dealing with > delegates, creating the fetchers). > And in the end, i'm not sure it's truely a factory -- the creation is still > forwarded to the factory we inherit from. And the test doesn't want to deal with > a factory -- we just want to inject our fetchers somehow. > > I'm not saying we can't do it like you propose, but I'd like to understand why > we need to deal with all the details (which have a good potential for bugs) ;-) It's true that we deal with a lot of implementation details. The reason here is, that we never tested for the correct configuration of the fetcher before. Sadly, this is nothing, we could see just by interacting with the life of a request. And I also agree that it's tricky to see the real issue here. The tests don't want to deal with a factory, but they have to use one for injecting the fetchers. These fetchers all have the same ID. We need to identify fetchers by ID to call a callback. Calling every callback is probably the only things that tests want to deal less with: 1. It's not connected to the tests themselves. 2. Unrelated to the real use case, where you don't have to know when this happens. 3. If you forget to do so (which is more than likely), the test will crash everything ASan-related. Automatically callback calling is complicated as it must not happen multiple times. Therefore, we have to track the calls. This is only possible we know of a fetcher's life time. https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:222: return; On 2016/12/12 10:17:16, Marc Treib wrote: > When would |fetcher| be null here? Add a comment to explain? Removed. It cannot be 0. If it is, crashing is an appropriate action. https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:288: struct ExpectationForVariationParam { On 2016/12/12 10:17:16, Marc Treib wrote: > nit: Move this into the test that uses it. Done. https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:1053: {"", 0}, // Keep 2 retries if no variation is set. On 2016/12/12 10:17:16, Marc Treib wrote: > Comment is out of date. > One alternative is adding a "description" string to each test case. Then when > one of them fails, we can print it, so it's clear what exactly failed. Good idea.
https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:1053: {"", 0}, // Keep 2 retries if no variation is set. On 2016/12/12 11:05:10, fhorschig wrote: > On 2016/12/12 10:17:16, Marc Treib wrote: > > Comment is out of date. > > One alternative is adding a "description" string to each test case. Then when > > one of them fails, we can print it, so it's clear what exactly failed. > > Good idea. This is fine by me, but we're getting close into the realms of discouraged data-driven tests. https://big.corp.google.com/~jmcmaster/testing/2008/07/episode-97-blast-from-... is an interesting read.
https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2548343002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:1053: {"", 0}, // Keep 2 retries if no variation is set. On 2016/12/12 11:55:48, tschumann wrote: > On 2016/12/12 11:05:10, fhorschig wrote: > > On 2016/12/12 10:17:16, Marc Treib wrote: > > > Comment is out of date. > > > One alternative is adding a "description" string to each test case. Then > when > > > one of them fails, we can print it, so it's clear what exactly failed. > > > > Good idea. > > This is fine by me, but we're getting close into the realms of discouraged > data-driven tests. > https://big.corp.google.com/~jmcmaster/testing/2008/07/episode-97-blast-from-... > is an interesting read. Looks like a valid concern even though this very episode was very controversially discussed: http://g/unittest-discuss/7bi0kIjCgak I added it to go/zine-testing and the agenda of go/zine-testing-notes. We should discuss where we draw the line. For this particular test, I see no huge problem, especially with the descriptions in place...
lgtm lgtm as discussed offline, let's document the missing functionality of the existing test factories (in a TODO) and also mention in the TODO() that we should add this behavior to the existing test classes instead of rolling our own.
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2548343002/#ps240001 (title: "Document missing functionality inline.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481552066091980,
"parent_rev": "75e4f77392623e854412ca26a132911cf58c3dc9", "commit_rev":
"7dac9d5093426afe74055db803a63ec392ca19fa"}
Message was sent while issue was closed.
Description was changed from ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority and their number will be determined by a variation parameter. This CL does not reschedule/retry non-interactive requests, as we figured that this small change improves the current state on its own and rescheduling requires some more discussion. BUG=668074 ========== to ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority and their number will be determined by a variation parameter. This CL does not reschedule/retry non-interactive requests, as we figured that this small change improves the current state on its own and rescheduling requires some more discussion. BUG=668074 Review-Url: https://codereview.chromium.org/2548343002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority and their number will be determined by a variation parameter. This CL does not reschedule/retry non-interactive requests, as we figured that this small change improves the current state on its own and rescheduling requires some more discussion. BUG=668074 Review-Url: https://codereview.chromium.org/2548343002 ========== to ========== Set MaxRetriesOn5xx only for interactive requests. Keep retries on 5xx errors only for user-triggered, interactive requests. Other fetches have no immediate priority and their number will be determined by a variation parameter. This CL does not reschedule/retry non-interactive requests, as we figured that this small change improves the current state on its own and rescheduling requires some more discussion. BUG=668074 Committed: https://crrev.com/6d88a9de178e49cfd9f2660d3ba1d7a1069c18ff Cr-Commit-Position: refs/heads/master@{#437872} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6d88a9de178e49cfd9f2660d3ba1d7a1069c18ff Cr-Commit-Position: refs/heads/master@{#437872} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
