Skip to content

Commit

Permalink
dcache-bulk: deprecate "prestore" option and remove related container
Browse files Browse the repository at this point in the history
Motivation:

`prestore` was added as a bulk request option originally in order
to allow immediate retrieval of target paths.   When it was then
discovered that lazy storing of the initial paths was not a
workable idea, this option applied only to targets discovered
via recursion.

However, the rationale for storing the entire tree of targets before
beginning the processing does not really exist.  The lazy storage of
discovered targets, and the deferring of directories in memory
until they are handled, which is the behavior of the default
container, does not pose problems for recovery, because the tree
is rescanned in that case, and only the stored targets which have
successfully completed or failed are skipped; hence, the entire
directory list is reconstituted on restart for any requests that
had not been completed before shutdown of the service.

Moreover, time-to-completion is significantly increased (by at
least an order of magnitude) because of the additional overhead
involved in doing this on a sizeable tree of discovered targets.

Modification:

The option has been deprecated and all related code removed.
The presence of the `prestore` attribute in the JSON for
the request will simply be ignored (for backward compatibility).

Result:

The additional semantics, which complicate modifications
(should they be desirable), of prestorage are no longer
maintained.

`I would suggest we backport this to 9.1, where the major
database changes took place, but leaving it only for
9.2 is acceptable.`

Target: master
Request: 9.1
Patch: https://rb.dcache.org/r/14035
Acked-by: Tigran
  • Loading branch information
alrossi committed Aug 2, 2023
1 parent 5f5a5a9 commit 9f6910e
Show file tree
Hide file tree
Showing 12 changed files with 51 additions and 463 deletions.
26 changes: 25 additions & 1 deletion docs/TheBook/src/main/markdown/config-bulk.md
Expand Up @@ -163,4 +163,28 @@ is a clean-slate retry of the request).
Particularly useful are the following policy properties which can be changed without
requiring service restart using the admin shell command ``request policy``:

![Bulk Request Policy Command](images/bulk-request-policy.png)
![Bulk Request Policy Command](images/bulk-request-policy.png)

## Deprecation of the "prestore" option

As of 9.2, the `prestore` semantics (option and container type) have been eliminated.

There are two principal reasons for this.

First, it is only relevant for recursive requests. This is because
originally the initial targets were not automatically stored. This
was discovered to be incorrect behavior. Once initial targets are
stored immediately, the rationale for storing the entire tree of
targets before beginning the processing no longer makes much sense.

Secondly, the time-to-completion is significantly increased (by at
least an order of magnitude) because of the additional overhead
involved in doing this on a sizeable tree of discovered targets.

The lazy storage of discovered targets, and the deferring of directories
in memory until they are handled, which is the behavior of the default
container, does not pose problems for recovery, because the tree
is rescanned in that case, and only the stored targets which have
successfully completed or failed are skipped; hence, the entire
directory list is reconstituted on restart for any requests that
had not been completed before shutdown of the service.
15 changes: 14 additions & 1 deletion docs/UserGuide/src/main/markdown/frontend.md
Expand Up @@ -2729,4 +2729,17 @@ curl -k -L -H "Roles: admin" -H "Authorization: Bearer ${TOKEN}" -X POST "http

The `Roles` header takes a comma-delimited list of available roles the user wishes to assert.
Whether those roles are assigned, of course, depends upon whether the user has actually
been authorized to have them.
been authorized to have them.

## Deprecation of the "prestore" option

With 9.2, this option is no longer supported, for several reasons.
See The Book, CHAPTER 18. THE BULK SERVICE, for an explanation.

Note that this change has no effect on the handling of flat/non-recursive
requests such as those specified by the TAPE REST API.

From the standpoint of the JSON bulk description, we still allow
for the presence of `prestore`/`preStore`/`pre_store`, etc. so
that current scripts (if any) do not need to change. But the
option will simply be discarded by the frontend resource.
Expand Up @@ -122,11 +122,11 @@ public final class BulkServiceCommands implements CellCommandListener {
private static final Logger LOGGER = LoggerFactory.getLogger(BulkServiceCommands.class);

/**
* id | arrived | started | modified | owner | activity | depth | prestore | status :
* id | arrived | started | modified | owner | activity | depth | status :
* urlPrefix/uid
*/
private static final String FORMAT_REQUEST_FULL
= "%-12s | %-19s | %19s | %19s | %12s | %15s | %7s | %11s | %8s | %s/%s";
= "%-12s | %-19s | %19s | %19s | %12s | %15s | %7s | %8s | %s/%s";

/**
* (urlPrefix/uid): target string
Expand Down Expand Up @@ -357,7 +357,6 @@ private static String formatRequest(BulkRequest request,
request.getActivity(),
request.getExpandDirectories(),
statusName,
request.isPrestore(),
request.getUrlPrefix(),
uid);
case TARGET:
Expand Down Expand Up @@ -451,11 +450,6 @@ abstract class FilteredRequest implements Callable<Serializable> {
usage = "The recursion depth of the request.")
String expandDirectories;

@Option(name = "prestore",
usage = "True means the request has indicated that all targets be stored "
+ "first before processing.")
Boolean prestore;

@Option(name = "status",
valueSpec = "QUEUED|STARTED|COMPLETED|CANCELLED",
separator = ",",
Expand Down Expand Up @@ -493,7 +487,7 @@ protected void configureFilters() throws ParseException {

rFilter = new BulkRequestFilter(beforeStart, afterStart, owners, urlPrefixes, ids,
activities, statuses, cancelOnFailure, clearOnSuccess, clearOnFailure, delayClear,
depth, prestore);
depth);
rFilter.setId(id);
}

Expand Down Expand Up @@ -961,13 +955,6 @@ class RequestSubmit implements Callable<String> {
usage = "Remove request from storage if all targets succeeded.")
Boolean clearOnSuccess = false;

@Option(name = "prestore",
usage =
"Store all targets first before performing the activity on them. (This applies "
+ "to recursive as well as non-recursive, and usually results in significantly "
+ "lower throughput.)")
Boolean prestore = false;

@Option(name = "arguments",
usage = "Optional comma-delimited list of name:value strings specific to the activity.")
String arguments;
Expand All @@ -985,7 +972,6 @@ public String call() {
request.setClearOnFailure(clearOnFailure);
request.setExpandDirectories(Depth.valueOf(expand.toUpperCase()));
request.setUid(UUID.randomUUID().toString());
request.setPrestore(activity.equalsIgnoreCase("STAGE") || prestore);

if (arguments != null) {
request.setArguments(Splitter.on(',')
Expand Down

0 comments on commit 9f6910e

Please sign in to comment.