CDAP-20884: Multi push operation

[GnsP]

Implement Push LRO

• Fetches applications one by one from db/api and writes it to local repo
• The operation is atomic i.e the push happens after all app changes has been committed ( in one commit)

[chtyim]

Always retain the original exception as the cause when rethrowing.

[GnsP]

updated.

[chtyim]

Define a class for the result instead of Map<Path, String> to better represent what the result is and easier to include more information in future.

[chtyim]

Seems like should have a batch getFileHashes method to do it more efficiently by doing a single TreeWalk instead of scanning it multiple times.

[GnsP]

updated. Added a new method getFileHashes to get the hashes using a single TreeWalk.

[chtyim]

Typically when wrapping an exception, you don't use the e.getMessage() to form the wrapping message, otherwise it just result it duplicated message in the stacktrace.

[GnsP]

updated.

[albertshau]

this is a problem in the existing PullAppsOperation too, but OperationException should take another exception as the cause. As it is now, the root cause will be lost.

[GnsP]

updated the OperationException class constructor to take the cause too. Updated the usage in PullAppsOperation.

[albertshau]

nit: change to > 1 for consistency with rest of the project

[GnsP]

done.

[albertshau]

can this actually happen? If so, when? Would be good to add a comment

[GnsP]

This exception should never happen, as the getName method on jgit.lib.ObjectId always returns a non-null string. Removed this check (unnecessary).

[albertshau]

if a Path in the input doesn't actually exist, what is the expected behavior? Does it throw an exception? Is it just not returned in the Map? In the Map but with empty file hash? Should add a javadoc describing the behavior, and also when each type of exception is thrown.

[GnsP]

If a Path in the input does not actually exist, then it's not included in the Map at all. Only the paths that are found in the filtered TreeWalk are added to the Map.

This method does not throw any exception in this case. This case is handled in the method that uses it (i.e. the commitAndPush method). If the returned map has different number of entries than expected (based on the input), then a SourceControlException is thrown.

[albertshau]

please add this information as a javadoc for the method.

[albertshau]

I'm not familiar with the git library we're using, but I feel like this would likely be implemented in an inefficient way, where it does N comparisons for each element in the walk.

It would be more efficient and simpler (don't need the if-else) to create a PathFilter that just checks if the path is in the input set with Set.contains().

[GnsP]

I checked the library implementation. Yes, it does N comparisons for each element in the walk.

Updated the method to filter the paths during the tree traversal based on their existence in the set of relative paths.

[albertshau]

does this clone into the base directory? Confused why we need to create the directory later if it was already cloned here.

[GnsP]

The repository root directory (this gets cloned) and the scm base directory are different. The scm base directory is a directory inside the repository root, configurable per namespace. This base directory may not exist if the user is pushing apps from the namespace for the first time. That's why we need to create it, if it does not exist already.

[albertshau]

this can be a really long log line, it's better to just log the number of apps that will be pushed. If we want to log each one, have a separate log statement within the loop below.

[GnsP]

done

[albertshau]

In this method, we're creating this Map, another gitFileHashes Map, and a new HashSet that's passed into commitAndPush(). That's a lot of extra passes through the data, a bit of extra memory, and extra mapping complexity that makes the method harder to understand. If the code needs to do a bunch of converting back and forth of collections, it's an indication that the APIs probably need improvement.

For example, this could all be done in one pass if we change commitAndPush() to look like:

public <T extends Supplier<Path>> void commitAndPush(Iterator<T> commitFiles, BiConsumer<T, String> consumer);

and call it something like:

private static class PushFile implements Supplier<Path> {
  private final AppVersion appVersion;
  private final Path path;

  public Path get() {
    return path;
  }
}

Iterator<PushFile> iter = Iterators.transform(appsToPush, appToPush -> new PushFile(appToPush, getFilePath(...));
repositoryManager.commitAndPush(iter, (pushFile, fileHash) -> response.add(new PushAppResponse(...)));

[GnsP]

updated.

[albertshau]

can change the entire method to call multiPush() and return the first response instead of just this section.

[GnsP]

changing the push method to use multiPush internally does not simplify the implementation, as these 2 methods are logically different (by design).

In case of single application push method, the request object (of type PushAppOperationRequest) already contains the ApplicationDetail of the app to push.

However, the multiPush method takes a request containing a set of app names, and an ApplicationManager. The applicationManager fetches the application details when necessary.

[albertshau]

minor comments

[albertshau]

should pass e as the cause. When wrapping an exception we will basically always want to set the cause if there is one.

[GnsP]

done

[albertshau]

please add this information as a javadoc for the method.

[albertshau]

nit: a little more readable to reduce one layer of nesting like:

if (walk.isSubtree()) {
  ...
  continue;
}
Path filePath...

[GnsP]

done

[albertshau]

the previous comment was about changing the RepositoryManager.commitAndPush() method to take the Iterator and BiConsumer. That way this method doesn't need to create the extra collections like the current pathMapping Map.

[GnsP]

The RepositoryManager.commitAndPush needs to construct a set of paths from the iterator anyway, as the set is necessary to efficiently filter the files in the TreeWalk. Also, it makes sense for the RepositoryManager.commitAndPush to return a Map of filepaths to hashes (as it's one operation, that adds multiple files to a commit and pushes) instead of taking a consumer to pass the results one by one. Therefore, updating this method did not improve readability much.

However, the suggested change made perfect sense for the commitAndPush method in the runner, as it simplified the implementation of this method.

[albertshau]

The original comment was mostly about avoiding all of these extra collections, as they add time, memory, and complexity. I don't think it's actually good that commitAndPush returns a Map, as it almost always forces the caller to eventually transform it into some other type of collection.

If we want it to return information about what it committed instead of passing in a consumer, a better approach would be for it to return a Collection<O> and to take a BiFunction<T, String, O>, where O would be a PushAppResponse in this class. I think the current implementation of commitAndPush would make that difficult, but see the other comment I made about getting the file hash right when adding the file instead of walking through the tree after committing.

[albertshau]

Is it possible to get the file hash from whatever this returns? I see there is a DirCache.getEntry().getObjectId(), but not sure if that's the same as what's happening in getFileHashes.

If so it would be better than doing the tree walk later, and would allow passing in an iterator of paths instead of the Set.

[samdgupi]

Checked in test and looks like the hashes are the same. We can skip the tree walk. Thanks for the suggestion @albertshau
@GnsP Please do a reverify before making the changes

[samdgupi]

Updated the commitandpush take a BiFunction<T, String, O> and return Collection<O>. Updated all the test accordingly

[albertshau]

The original comment was mostly about avoiding all of these extra collections, as they add time, memory, and complexity. I don't think it's actually good that commitAndPush returns a Map, as it almost always forces the caller to eventually transform it into some other type of collection.

If we want it to return information about what it committed instead of passing in a consumer, a better approach would be for it to return a Collection<O> and to take a BiFunction<T, String, O>, where O would be a PushAppResponse in this class. I think the current implementation of commitAndPush would make that difficult, but see the other comment I made about getting the file hash right when adding the file instead of walking through the tree after committing.

[chtyim]

Missing doc for hashConsumer and the @return

[GnsP]

Updated the javadoc

[albertshau]

minor comment, otherwise lgtm

[albertshau]

should use an ArrayList. It basically always performs better than a LinkedList, even if it needs to get resized underneath. This is especially true when we know the size beforehand (like we do here) and can pass it in to the constructor.

[GnsP]

Updated.

[chtyim]

debug log

[GnsP]

updated.

[chtyim]

trace log

[GnsP]

updated.

[chtyim]

This loop would be very slow? It is getting one app at a time from the worker remotely? We should add a batch method to the manager.

[GnsP]

Currently this operation uses only the LocalApplicationManager, which gets the ApplicationDetail in memory. The RemoteApplicationManager will be used here, only when the corresponding LongRunningOperationRunner is implemented. @samdgupi can provide more information on it.

[chtyim]

That’s why the implementation here is not safe. We should have a batch fetch method

[samdgupi]

AFAIK We currently have no API/ which can fetch details by list of ids. We had plan to do this optimization after initial release of the feature. I will add this a TODO and take it up if possible as part of current release.