From 2944ba1b903de5907f03729b8a551509bd33c6a2 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 10 Jun 2020 19:44:57 +0200 Subject: [PATCH 1/3] ProjectJson: Use merge function for label value rendering Normally label values must be unique, so that merger function shouldn't be passed in to the Collectors.toMap() function, but we got report that indicates that the mapped keys (label values) might have duplicates. Even though this change only treats symptoms of the problem but not the real cause, it could help to identify why the mapped keys were not unique in the first place. Change-Id: I9d1a8d7157598001daf4b50afd16ca2d2e5f9553 --- .../google/gerrit/server/project/ProjectJson.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/project/ProjectJson.java b/java/com/google/gerrit/server/project/ProjectJson.java index f2a93d33d9..449f6073f2 100644 --- a/java/com/google/gerrit/server/project/ProjectJson.java +++ b/java/com/google/gerrit/server/project/ProjectJson.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.project; import static java.util.stream.Collectors.toMap; import com.google.common.base.Strings; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelValue; import com.google.gerrit.extensions.common.LabelTypeInfo; @@ -33,6 +34,7 @@ import java.util.List; @Singleton public class ProjectJson { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final AllProjectsName allProjects; private final WebLinks webLinks; @@ -49,7 +51,17 @@ public class ProjectJson { for (LabelType t : projectState.getLabelTypes().getLabelTypes()) { LabelTypeInfo labelInfo = new LabelTypeInfo(); labelInfo.values = - t.getValues().stream().collect(toMap(LabelValue::formatValue, LabelValue::getText)); + t.getValues().stream() + .collect( + toMap( + LabelValue::formatValue, + LabelValue::getText, + (v1, v2) -> { + logger.atSevere().log( + "Duplicate values for project: %s, label: %s found: '%s':'%s'", + projectState.getName(), t.getName(), v1, v2); + return v1; + })); labelInfo.defaultValue = t.getDefaultValue(); info.labels.put(t.getName(), labelInfo); } From 002c03086e3eafb02a8e5db4243abce75d4ada1d Mon Sep 17 00:00:00 2001 From: Orgad Shaneh Date: Sun, 7 Jun 2020 13:56:27 +0300 Subject: [PATCH 2/3] Schema: Refactor lambda in buildFields to a separate function Also do a little of the refactoring done later in d03143571d2ef104a0c306e0e1c1831e7adb7958. Change-Id: I70110e998d2b69a414df5bee0d97fe5732558af4 (cherry picked from commit 2ba8153d68812d6cad1c2bee2fc96acbcc17cd88) --- java/com/google/gerrit/index/Schema.java | 49 +++++++++++------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/java/com/google/gerrit/index/Schema.java b/java/com/google/gerrit/index/Schema.java index 18563ab6cd..215e22ac9e 100644 --- a/java/com/google/gerrit/index/Schema.java +++ b/java/com/google/gerrit/index/Schema.java @@ -15,11 +15,9 @@ package com.google.gerrit.index; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; -import com.google.common.base.Function; import com.google.common.base.MoreObjects; -import com.google.common.base.Predicates; -import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.flogger.FluentLogger; @@ -28,6 +26,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Objects; import java.util.Optional; /** Specific version of a secondary index schema. */ @@ -165,6 +164,23 @@ public class Schema { return true; } + private Values fieldValues(T obj, FieldDef f) { + Object v; + try { + v = f.get(obj); + } catch (OrmException e) { + logger.atSevere().withCause(e).log("error getting field %s of %s", f.getName(), obj); + return null; + } + if (v == null) { + return null; + } else if (f.isRepeatable()) { + return new Values<>(f, (Iterable) v); + } else { + return new Values<>(f, Collections.singleton(v)); + } + } + /** * Build all fields in the schema from an input object. * @@ -174,29 +190,10 @@ public class Schema { * @return all non-null field values from the object. */ public final Iterable> buildFields(T obj) { - return FluentIterable.from(fields.values()) - .transform( - new Function, Values>() { - @Override - public Values apply(FieldDef f) { - Object v; - try { - v = f.get(obj); - } catch (OrmException e) { - logger.atSevere().withCause(e).log( - "error getting field %s of %s", f.getName(), obj); - return null; - } - if (v == null) { - return null; - } else if (f.isRepeatable()) { - return new Values<>(f, (Iterable) v); - } else { - return new Values<>(f, Collections.singleton(v)); - } - } - }) - .filter(Predicates.notNull()); + return fields.values().stream() + .map(f -> fieldValues(obj, f)) + .filter(Objects::nonNull) + .collect(toImmutableList()); } @Override From 061f3b238a65e9ee52fb335ce7a83372ebaea9cb Mon Sep 17 00:00:00 2001 From: Orgad Shaneh Date: Sun, 7 Jun 2020 13:56:27 +0300 Subject: [PATCH 3/3] Schema: Show only a single log for inexistent commits Producing 16 logs for each commit is pointless. Finding an OrmException means we're going to hit exceptions again and again for each field, so bubble them up and return an empty list when we find one. Example (unmodified from cherry-pick source where OrmException has been renamed to StorageException): [2020-06-07T13:28:28.346+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field added of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} com.google.gerrit.exceptions.StorageException: Loading commit AnyObjectId[cee21a4ec8b5ae2469f7f314c2548dedc1023293] for ps 2 of change 777 failed. at com.google.gerrit.server.query.change.ChangeData.loadCommitData(ChangeData.java:596) at com.google.gerrit.server.query.change.ChangeData.getDiffSummary(ChangeData.java:398) at com.google.gerrit.server.query.change.ChangeData.computeChangedLines(ChangeData.java:414) [2020-06-07T13:28:28.352+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field author of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.355+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field committer of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.357+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field message of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.358+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field deleted of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.360+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field delta of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.361+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field directory of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.364+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field exactauthor of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.365+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field exactcommitter of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.367+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field extension of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.375+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field filepart of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.377+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field footer of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.379+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field onlyextensions of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.380+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field file of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.388+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field tr of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} [2020-06-07T13:28:28.390+0300] [Index-Batch-1] ERROR com.google.gerrit.index.Schema : error getting field merge of ChangeData{Change{777 (Ia3439cd1765c9a7a08a07a1485fbc95aabd8cf56), dest=Project,refs/heads/master, status=M}} Change-Id: If23916d20134c869999d2588316936f29b0701db (cherry picked from commit 7c7b8d86a112bec31d51eed384477b93e7cae20e) --- java/com/google/gerrit/index/Schema.java | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/java/com/google/gerrit/index/Schema.java b/java/com/google/gerrit/index/Schema.java index 215e22ac9e..e5829c0aea 100644 --- a/java/com/google/gerrit/index/Schema.java +++ b/java/com/google/gerrit/index/Schema.java @@ -169,6 +169,13 @@ public class Schema { try { v = f.get(obj); } catch (OrmException e) { + // OrmException is thrown when the object is not found. On this case, + // it is pointless to make further attempts for each field, so propagate + // the exception to return an empty list. + logger.atSevere().withCause(e).log("error getting field %s of %s", f.getName(), obj); + throw new RuntimeException( + e); // work around throwing checked exceptions from methods used in Streams + } catch (RuntimeException e) { logger.atSevere().withCause(e).log("error getting field %s of %s", f.getName(), obj); return null; } @@ -190,10 +197,17 @@ public class Schema { * @return all non-null field values from the object. */ public final Iterable> buildFields(T obj) { - return fields.values().stream() - .map(f -> fieldValues(obj, f)) - .filter(Objects::nonNull) - .collect(toImmutableList()); + try { + return fields.values().stream() + .map(f -> fieldValues(obj, f)) + .filter(Objects::nonNull) + .collect(toImmutableList()); + } catch (RuntimeException e) { + if (e.getCause().getClass().equals(OrmException.class)) { + return ImmutableList.of(); + } + throw e; + } } @Override