Add a logs field in REST API returned dbgmsgs.
* These trace messages will help debugging Gerrit server side bug
from find-owners client request.
* logs are included in every OwnersDb object but returned to
client only for requests with debug=1 parameter.
* Add a 'nocache' parameter to compute OwnersDb without using the cache.
Change-Id: I1a62e2cf2a485aeb194e2aaec6bd546d307f32c8
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
index d0c642f..493ae7e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -67,6 +67,7 @@
static class Parameters {
Boolean debug; // REST API "debug" parameter, or null
+ Boolean nocache; // REST API "nocache" parameter, or null
Integer patchset; // REST API "patchset" parameter, or null
}
@@ -104,7 +105,7 @@
private List<OwnerInfo> getOwners(OwnersDb db, Collection<String> files) {
Map<String, OwnerWeights> weights = new HashMap<>();
- db.findOwners(files, weights);
+ db.findOwners(files, weights, new ArrayList<>());
List<OwnerInfo> result = new ArrayList<>();
Set<String> emails = new HashSet<>();
for (String key : OwnerWeights.sortKeys(weights)) {
@@ -182,9 +183,9 @@
throws OrmException, BadRequestException, IOException {
int patchset = getValidPatchsetNum(changeData, params.patchset);
ProjectState projectState = projectCache.get(changeData.project());
- OwnersDb db =
- Cache.getInstance()
- .get(projectState, accountCache, emails, repository, changeData, patchset);
+ Boolean useCache = params.nocache == null || !params.nocache;
+ OwnersDb db = Cache.getInstance().get(
+ useCache, projectState, accountCache, emails, repository, changeData, patchset);
Collection<String> changedFiles = changeData.currentFilePaths();
Map<String, Set<String>> file2Owners = db.findOwners(changedFiles);
@@ -201,6 +202,7 @@
obj.dbgmsgs.errors = db.errors;
obj.dbgmsgs.path2owners = Util.makeSortedMap(db.path2Owners);
obj.dbgmsgs.owner2paths = Util.makeSortedMap(db.owner2Paths);
+ obj.dbgmsgs.logs = db.logs;
}
obj.file2owners = Util.makeSortedMap(file2Owners);
@@ -236,6 +238,7 @@
OwnersDb db =
Cache.getInstance()
.get(
+ true, // use cached OwnersDb
projectCache.get(resource.getProject()),
accountCache,
emails,
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
index f1a2bee..bf01666 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
@@ -88,6 +88,7 @@
/** Returns a cached or new OwnersDb, for the current patchset. */
OwnersDb get(
+ Boolean useCache,
ProjectState projectState,
AccountCache accountCache,
Emails emails,
@@ -95,6 +96,7 @@
ChangeData changeData)
throws OrmException, IOException {
return get(
+ useCache,
projectState,
accountCache,
emails,
@@ -105,6 +107,7 @@
/** Returns a cached or new OwnersDb, for the specified patchset. */
OwnersDb get(
+ Boolean useCache,
ProjectState projectState,
AccountCache accountCache,
Emails emails,
@@ -116,6 +119,7 @@
String dbKey = Cache.makeKey(changeData.getId().get(), patchset, branch);
// TODO: get changed files of the given patchset?
return get(
+ useCache,
projectState,
accountCache,
emails,
@@ -128,6 +132,7 @@
/** Returns a cached or new OwnersDb, for the specified branch and changed files. */
OwnersDb get(
+ Boolean useCache,
ProjectState projectState,
AccountCache accountCache,
Emails emails,
@@ -136,7 +141,7 @@
ChangeData changeData,
String branch,
Collection<String> files) {
- if (dbCache == null) { // Do not cache OwnersDb
+ if (dbCache == null || !useCache) { // Do not cache OwnersDb
logger.atFiner().log("Create new OwnersDb, key=%s", key);
return new OwnersDb(
projectState, accountCache, emails, key, repository, changeData, branch, files);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
index 2d71283..e361f82 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -151,7 +151,7 @@
// One update to a Gerrit change can call submit_rule or submit_filter
// many times. So this function should use cached values.
OwnersDb db =
- Cache.getInstance().get(projectState, accountCache, emails, repository, changeData);
+ Cache.getInstance().get(true, projectState, accountCache, emails, repository, changeData);
if (db.getNumOwners() <= 0) {
return 0;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
index c62a909..5d19399 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
@@ -46,6 +46,10 @@
@Option(name = "--debug", usage = "get extra debug info")
private String debug;
+ // "nocache" could be true/yes/1 or false/no/0, default is false.
+ @Option(name = "--nocache", usage = "do not use cached owners info")
+ private String nocache;
+
@Option(name = "--patchset", usage = "select change patchset number")
private Integer patchset;
@@ -78,6 +82,7 @@
Action.Parameters params = new Action.Parameters();
params.patchset = patchset;
params.debug = (debug != null) ? Util.parseBoolean(debug) : null;
+ params.nocache = (nocache != null) ? Util.parseBoolean(nocache) : null;
try {
return this.action.apply(rsrc, params);
} catch (BadRequestException e) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
index 227369a..edd1a0a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -23,11 +23,14 @@
import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.PathMatcher;
import java.nio.file.Paths;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
@@ -58,6 +61,7 @@
Set<String> stopLooking = new HashSet<>(); // directories where OWNERS has "set noparent"
Map<String, String> preferredEmails = new HashMap<>(); // owner email to preferred email
List<String> errors = new ArrayList<>(); // error messages
+ List<String> logs = new ArrayList<>(); // trace/debug messages
OwnersDb() {}
@@ -73,10 +77,18 @@
this.accountCache = accountCache;
this.emails = emails;
this.key = key;
+ try {
+ InetAddress inetAddress = InetAddress.getLocalHost();
+ logs.add("HostName:" + inetAddress.getHostName());
+ } catch (UnknownHostException e) {
+ logException(logs, "HostName:", e);
+ }
+ logs.add("key:" + key);
preferredEmails.put("*", "*");
String ownersFileName = Config.getOwnersFileName(projectState, changeData);
+ logs.add("ownersFileName:" + ownersFileName);
// Some hacked CL could have a target branch that is not created yet.
- ObjectId id = getBranchId(repository, branch, changeData);
+ ObjectId id = getBranchId(repository, branch, changeData, logs);
revision = "";
if (id != null) {
for (String fileName : files) {
@@ -84,10 +96,12 @@
// Stop looking for a parent directory if OWNERS has "set noparent".
fileName = Util.normalizedFilePath(fileName);
String dir = Util.normalizedDirPath(fileName); // e.g. dir = ./d1/d2
+ logs.add("findOwnersFileFor:" + fileName);
while (!readDirs.contains(dir)) {
readDirs.add(dir);
+ logs.add("findOwnersFileIn:" + dir);
String filePath = (dir + "/" + ownersFileName).substring(2); // remove "./"
- String content = getRepositoryFile(repository, id, filePath);
+ String content = getRepositoryFile(repository, id, filePath, logs);
if (content != null && !content.equals("")) {
addFile(dir + "/", dir + "/" + ownersFileName, content.split("\\R+"));
}
@@ -102,6 +116,7 @@
} catch (Exception e) {
logger.atSevere().withCause(e).log(
"Fail to get branch revision for %s", Config.getChangeId(changeData));
+ logException(logs, "OwnersDb get revision", e);
}
}
countNumOwners(files);
@@ -112,7 +127,8 @@
}
private void countNumOwners(Collection<String> files) {
- Map<String, Set<String>> file2Owners = findOwners(files, null);
+ logs.add("countNumOwners");
+ Map<String, Set<String>> file2Owners = findOwners(files, null, logs);
if (file2Owners != null) {
Set<String> emails = new HashSet<>();
file2Owners.values().forEach(emails::addAll);
@@ -141,6 +157,7 @@
email2ids = emails.getAccountsFor(ownerEmailsAsArray);
} catch (Exception e) {
logger.atSevere().withCause(e).log("accounts.byEmails failed");
+ logException(logs, "getAccountsFor:" + ownerEmailsAsArray[0], e);
}
for (String owner : ownerEmailsAsArray) {
String email = owner;
@@ -192,8 +209,10 @@
ArrayList<Integer> distances,
String file,
Map<String, Set<String>> file2Owners,
- Map<String, OwnerWeights> map) {
+ Map<String, OwnerWeights> map,
+ List<String> logs) {
for (int i = 0; i < paths.size(); i++) {
+ logs.add("addOwnerWeightsIn:" + paths.get(i));
Set<String> owners = path2Owners.get(paths.get(i));
if (owners == null) {
continue;
@@ -214,13 +233,13 @@
/** Quick method to find owner emails of every file. */
Map<String, Set<String>> findOwners(Collection<String> files) {
- return findOwners(files, null);
+ return findOwners(files, null, new ArrayList<>());
}
/** Returns owner emails of every file and set up ownerWeights. */
Map<String, Set<String>> findOwners(
- Collection<String> files, Map<String, OwnerWeights> ownerWeights) {
- return findOwners(files.toArray(new String[0]), ownerWeights);
+ Collection<String> files, Map<String, OwnerWeights> ownerWeights, List<String> logs) {
+ return findOwners(files.toArray(new String[0]), ownerWeights, logs);
}
/** Returns true if path has '*' owner. */
@@ -238,14 +257,18 @@
}
/** Returns owner emails of every file and set up ownerWeights. */
- Map<String, Set<String>> findOwners(String[] files, Map<String, OwnerWeights> ownerWeights) {
+ Map<String, Set<String>> findOwners(
+ String[] files, Map<String, OwnerWeights> ownerWeights, List<String> logs) {
// Returns a map of file to set of owner emails.
// If ownerWeights is not null, add to it owner to distance-from-dir;
// a distance of 1 is the lowest/closest possible distance
// (which makes the subsequent math easier).
+ logs.add("findOwners");
+ Arrays.sort(files); // Force an ordered search sequence.
Map<String, Set<String>> file2Owners = new HashMap<>();
for (String fileName : files) {
fileName = Util.normalizedFilePath(fileName);
+ logs.add("checkFile:" + fileName);
String dirPath = Util.normalizedDirPath(fileName);
String baseName = fileName.substring(dirPath.length() + 1);
int distance = 1;
@@ -257,6 +280,7 @@
boolean foundStar = false;
while (true) {
int savedSizeOfPaths = paths.size();
+ logs.add("checkDir:" + dirPath);
if (dir2Globs.containsKey(dirPath + "/")) {
Set<String> patterns = dir2Globs.get(dirPath + "/");
for (String pat : patterns) {
@@ -287,40 +311,59 @@
dirPath = Util.getDirName(dirPath); // go up one level
}
if (!foundStar) {
- addOwnerWeights(paths, distances, fileName, file2Owners, ownerWeights);
+ addOwnerWeights(paths, distances, fileName, file2Owners, ownerWeights, logs);
+ } else {
+ logs.add("found * in:" + fileName);
}
}
return file2Owners;
}
/** Returns ObjectId of the given branch, or null. */
- private static ObjectId getBranchId(Repository repo, String branch, ChangeData changeData) {
+ private static ObjectId getBranchId(
+ Repository repo, String branch, ChangeData changeData, List<String> logs) {
+ String header = "getBranchId:" + branch;
try {
ObjectId id = repo.resolve(branch);
if (id == null && changeData != null && !Checker.isExemptFromOwnerApproval(changeData)) {
logger.atSevere().log(
"cannot find branch %s for %s", branch, Config.getChangeId(changeData));
+ logs.add(header + " (NOT FOUND)");
+ } else {
+ logs.add(header + " (FOUND)");
}
return id;
} catch (Exception e) {
logger.atSevere().withCause(e).log(
"cannot find branch %s for %s", branch, Config.getChangeId(changeData));
+ logException(logs, header, e);
}
return null;
}
/** Returns file content or empty string; uses Repository. */
- private static String getRepositoryFile(Repository repo, ObjectId id, String file) {
+ private static String getRepositoryFile(
+ Repository repo, ObjectId id, String file, List<String> logs) {
+ String header = "getRepositoryFile:" + file;
try (RevWalk revWalk = new RevWalk(repo)) {
RevTree tree = revWalk.parseCommit(id).getTree();
ObjectReader reader = revWalk.getObjectReader();
TreeWalk treeWalk = TreeWalk.forPath(reader, file, tree);
if (treeWalk != null) {
- return new String(reader.open(treeWalk.getObjectId(0)).getBytes(), UTF_8);
+ String content = new String(reader.open(treeWalk.getObjectId(0)).getBytes(), UTF_8);
+ logs.add(header + ":" + content);
+ return content;
}
+ logs.add(header + " (NOT FOUND)");
} catch (Exception e) {
logger.atSevere().withCause(e).log("get file %s", file);
+ logException(logs, header, e);
}
return "";
}
+
+ /** Adds a header + exception message to the logs. */
+ private static void logException(List<String> logs, String header, Exception e) {
+ logs.add(header + " Exception:" + e.getMessage());
+ }
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
index 15f01de..378f2de 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
@@ -57,5 +57,6 @@
List<String> errors;
SortedMap<String, List<String>> path2owners;
SortedMap<String, List<String>> owner2paths;
+ List<String> logs;
}
}
diff --git a/src/main/resources/Documentation/rest-api.md b/src/main/resources/Documentation/rest-api.md
index 10b1565..d287480 100644
--- a/src/main/resources/Documentation/rest-api.md
+++ b/src/main/resources/Documentation/rest-api.md
@@ -19,6 +19,8 @@
* **debug**: can be set to true or false to override the configuration variable
**addDebugMsg**.
+* **nocache**: can be set to true to collect owerns info without using the cached OwnersDb.
+
For example,
```bash
@@ -66,6 +68,9 @@
a map from owner email to an array of directory path or file glob.
This is opposite to the path2owners map.
+ * **logs**:
+ trace messages during the search of OWNERS files.
+
* **file2owners**: a map from each file path in the change patchset to
an array of the file's owner emails.
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
index 323a861..6198139 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -96,8 +96,11 @@
assertThat(getOwnersResponse(c1)).contains(ownersAX + ", files:[ OWNERS ]");
// Check all fields in response.
String expectedTail =
- "path2owners:{ ./:[ a@a, x@x ] }, owner2paths:{ a@a:[ ./ ], x@x:[ ./ ] } }"
- + ", file2owners:{ ./t.c:[ a@a, x@x ] }, reviewers:[], "
+ "path2owners:{ ./:[ a@a, x@x ] }, owner2paths:{ a@a:[ ./ ], x@x:[ ./ ] }, logs:[ "
+ + "key:12:1:master, ownersFileName:OWNERS, getBranchId:refs/heads/master(FOUND), "
+ + "findOwnersFileFor:./t.c, findOwnersFileIn:., getRepositoryFile:OWNERS:x@x\\na@a\\n, "
+ + "countNumOwners, findOwners, checkFile:./t.c, checkDir:., addOwnerWeightsIn:./ ] "
+ + "}, file2owners:{ ./t.c:[ a@a, x@x ] }, reviewers:[], "
+ ownersAX
+ ", files:[ t.c ] }";
assertThat(getOwnersDebugResponse(c2)).contains(expectedTail);
@@ -454,14 +457,15 @@
Repository repo = repoManager.openRepository(project);
Cache cache = Cache.getInstance().init(0, 0);
OwnersDb db =
- cache.get(projectCache.get(project), accountCache, emails, repo, r.getChange(), 1);
+ cache.get(true, projectCache.get(project), accountCache, emails, repo, r.getChange(), 1);
Checker c = new Checker(repo, r.getChange(), 1);
return c.findApproval(accountCache, db);
}
- // Remove '"' and space; replace '\n' with ' '; ignore unpredictable "owner_revision".
+ // Remove '"' and space; replace '\n' with ' '; ignore "owner_revision" and "HostName:*".
private static String filteredJson(String json) {
- return json.replaceAll("[\" ]*", "").replace('\n', ' ').replaceAll("owner_revision:[^ ]* ", "");
+ return json.replaceAll("[\" ]*", "").replace('\n', ' ').replaceAll("owner_revision:[^ ]* ", "")
+ .replaceAll("HostName:[^ ]*, ", "");
}
private static String filteredJson(RestResponse response) throws Exception {