Retrying of requests is currently implemented only for a subset of REST
endpoints (modifying change REST endpoints), but we would like to have
this functionality for all REST endpoints.
The retrying logic is currently specific to modifying change REST
endpoints since it's based on executing a ChangeAction with RetryHelper.
ChangeActions provide a BatchUpdate.Factory to their implementations,
but that's the only difference to the normal Action that can be retried.
If we use a normal Action for retrying requests (instead of a
ChangeAction) we can make the code that retries requests generic and use
it for all kind of REST endpoints. E.g. we could have one
RetryingRestModifyView class that can be used for all kind of modifying
REST endpoints instead of having one RetryingRestChangeModifyView for
change REST endpoints and one RetryingRestModifyView class for account,
group and project REST endpoints.
However implementing the retrying of requests in base classes of REST
endpoints also seems to be a bad idea since it means that retrying of
requests has to be implemented multiple times, once for each REST
endpoint interface that we have. At the moment we have 2 such base
classes, RetryingRestModifyView and RetryingRestCollectionModifyView. If
we would follow the current pattern we would need to add further base
classes, e.g. RetryingRestReadView, RetryingRestCollectionCreateView
etc.
This is why I want to remove these base classes and instead implement
the retrying of requests once directly in RestApiServlet.
Implementing the retrying of requests in RestApiServlet also has further
advantages:
- In case of auto-retry-with-trace we do not need a complicated way to
return the trace ID from the REST endpoint implementation back to
RestApiServlet (means we can simplify the existing code).
- In RestApiServlet we have more information about the REST endpoint
available and may include this information into traces on auto-retry.
Implementating all of this needs to touch a lot of classes. This is why
for easier reviews it's done in multiple steps:
1. Make retry logic in RetryingRestModifyView generic so that it can be
used for all kind of requests (this change):
This means we will provide a normal Action to the RetryHelper,
instead of a ChangeAction. The disadvantage of this is that now all
modifying change REST endpoints need to get the BatchUpdate.Factory
injected. But this doesn't seem to be worse than the current
requirement of injecting RetryHelper. Once the retrying is
implemented in RestApiServlet (see step 3) the modifying change REST
endpoints no longer need to get the RetryHelper injected. Hence after
all we just switch from injecting RetryHelper to injecting
BatchUpdate.Factory, which doesn't make a big difference. Some of the
REST endpoints that extended RetryingRestModifyView actually even
don't need a BatchUpdate.Factory, so for them the code actually gets
simpler.
We are now also using a dedicated action type for retrying REST
requests. This allows use to differentiate between retries of whole
requests vs retries of small actions when looking at the retry
metrics.
2. Make retry logic in RetryingRestCollectionModifyView generic so that
it can be used for all kind of requests.
3. Move the retrying logic into RestApiServlet:
This means we can now delete RetryingRestModifyView and
RetryingRestCollectionModifyView.
4. Remove code complications that were needed to return the trace ID
from a REST endpoint to RestApiServlet in case of
auto-retry-with-trace.
5. Enhance traces that are created on auto-retry:
Include information that is normally logged by RestApiServlet if a
request is traced (e.g. the REST endpoint URL, calling user, etc.)
6. Implement retry and auto-tracing for non-modifyable REST endpoints.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I3edd3673bf29428db2eb1ceac782da059ec58953
Signed-off-by: Edwin Kempin <ekempin@google.com>
d412bc8e2d