This sounds counter-intuitive, but this is feature compatible with the
GWT UI, which uses the GWT RPC interface.
Change-Id: I53a0f00684b33634883aaa9ecb7f2fd294456f5f
The race condition which exists for the change index (see I04ac36ea55a)
doesn't exist for the group index. Contrary to the change index, the
group index always uses the current value from the primary storage when
updating the index.
Let's assume that we have two concurrent updates of a group.
1. Updater A writes group in state A to primary storage.
2. Updater B writes group in state B to primary storage.
3. Updater B updates the index with state B from primary storage.
4. Updater A updates the index with state B from primary storage.
So, even when the writes to the storage and the updates of the index are
executed in an arbitrary order, the index still contains the correct
value in the end.
Change-Id: I1965b20c3d493ab05ec5d9636b9ac07f9ea3132b
Indexed groups are retrieved from the group cache. As group caches may
possibly have stale entries (for example when in slave mode), we have to
be careful not to use outdated values. All code paths which referred to
the GroupIndexer correctly invalidated the group cache. Requiring the
cache evictions in other classes is a bit dangerous, though.
This situation becomes even worse with the recently added
GroupIndexer#reindexIfStale method. Whether the group index is stale is
determined by comparing the group stored in the index with the one
stored in NoteDb. For updating a stale index entry, we didn't ensure
that the group was freshly read from NoteDb. This means that users of
GroupIndexer#reindexIfStale which care about up-to-date values would
need to manually invalidate the cache, which would trigger an indexing
without the stale check, and hence make the call to
GroupIndexer#reindexIfStale obsolete. Alternatively, they would need to
accept outdated values without any guarantee when they are updated.
To prevent future errors due to missing cache invalidations and to allow
calling GroupIndexer#index and GroupIndexer#reindexIfStale even with
stale caches (e.g. in slave mode), we explicitly use a non-cached value
for indexing now.
As the necessity for manual cache evictions is removed, it makes sense
in other parts of the code to not indirectly trigger an indexing for a
group by evicting it from the cache but to explicitly call the indexer.
As a result, evicting an element from the cache doesn't need to trigger
an indexing anymore. Previously, this was a bit confusing as it wasn't
explicitly documented in Javadoc and only happened for GroupCache#evict
and GroupCache#onCreateGroup but not for GroupCache#evictRename. It
would also have created an infinite loop for the new eviction in the
indexer.
In addition, grouping several evictions into GroupCache#evict made it
complicated to get a non-cached value from
GroupCache#get(AccountGroup.UUID). If just the group UUID was available,
this required that a group had to be loaded first from the cache via
GroupCache#get(AccountGroup.UUID), then evicted, and then loaded again.
As the first loading doesn't necessarily need to retrieve a cached
value, the group might have been loaded twice from storage.
For AllGroupsIndexer, there was even a possibly unintended side-effect:
The cache invalidation triggered an indirect indexing for the group
before the group was retrieved from the cache and explicitly put in the
index. The adjusted code should avoid this situation by only doing all
interaction with the index in AllGroupsIndexer.
We also use the opportunity to add some tests for the GroupIndexer and
the adjusted behavior.
Change-Id: I0b281d7b72831d78462f7a7ac5e57a67d09d980b
Nobody is making use of this. The only usage was removed by change
Id020fd8972. As explained in the commit message of change Id020fd8972 it
turned out that lazy parsing leads to a memory leak because for this the
reference to AccountConfig needs to be kept in memory and AccountConfig
may need quite a lot of memory since it holds an open Repository
instance.
Lazy parsing was not really needed and turned out to be a classic case
of premature optimization.
Change-Id: I534cfc0eeb01e0ccd4af3df3986fa23112f7cb63
Signed-off-by: Edwin Kempin <ekempin@google.com>
Google is the only user of this, and we no longer need this file to
change every time we import Gerrit.
Change-Id: I19e5238f236ec47c9893b74780cfe9768a1b5d78
DefaultRefFilter has a Map of visible changes as internal state. Reusing
this map in subsequent calls speeds the operation up.
Change-Id: Idb8627138badfeb3182b5904c3d569789334366c
Now that DefaultRefFilter is an implementation detail of
PermissionBackend we can remove READ_NO_CONFIG as it was only used to
migrate all callers away from direct calls to *Control classes.
Change-Id: I5bf0a6f59cf66b3720c2cb2b671d821eceb593bb
This commit adds signatures to filter refs by visibility to
PermissionBackend.ForProject.
VisibleRefFilter is renamed to DefaultRefFilter and moved into the
permissions package. Minor changes to the class make it fit better into
the new interface structure and make the class easier to read.
Instead of offering config options on the class, this commit adds an
Options AutoValue and forces callers to provide an open repository
handler to the filter method.
Change-Id: I1819de0bab81ed546f1c35461cc5361f1c4027ba
This class was implementing hashCode() but had no equals method.
Change-Id: Ib54862f2507eb4b2d54846e785ca9bd0cc561c16
Signed-off-by: Edwin Kempin <ekempin@google.com>
In I9b99490 AccountState was changed to lazily load the project watches
and preferences by passing a lambda referring to the AccountConfig
instance. Unfortunately, AccountConfig holds a reference to an open
Repository instance. In an implementation that does not cache these
instances, they are potentially expensive to hold, whether in terms of
heap or open resources. Plus, they are never closed.
Remove the lazy loading and go back to storing the watches and
preferences directly. This may be suboptimal memory usage, but in
practice will not be as bad as the many MiB of retained heap per entry
we're observing on googlesource.com. We still keep the Supplier
arguments in the AccountState constructor, to keep this change as simple
as possible to fix a production issue; reverting I9b99490 causes many
conflicts.
If we want to lazily load these fields, we will need to come up with an
approach that reopens the repository on demand. Alternatively, if we
decide that lazy loading is not worth the effort, we should remove the
Suppliers from the AccountState interface.
Change-Id: Id020fd89727b0a3377a98cffe8bf2e9cc2731451
All of the supported Git commands required the project to be writable.
We have seen HTTP 500s at Google because not all code paths would check
if the project state permits writes.
This commit moves the check in ReceiveCommits to a more central to
mitigate the problem.
Change-Id: Icdea3349583f676580991cd688ae7e9d95564a21
Being able to reindex a group only if it is stale can save effort on
startup of Gerrit slaves (see follow-up change).
Change-Id: I66d5bb09e49a4dfcd9d20db12c4ab818c94fd399
Signed-off-by: Edwin Kempin <ekempin@google.com>
String.split(String) has surprising behaviour [1]:
String[] nothing = "".split(":"); // results in [""]
String[] bunchOfNothing = ":".split(":"); // results in []
More examples:
input | input.split(":") | Splitter.on(':').split(input)
=======|===================|==============================
"" | [""] | [""]
":" | [] | ["", ""]
":::" | [] | ["", "", "", ""]
"a:::" | ["a"] | ["a", "", "", ""]
":::b" | ["", "", "", "b"] | ["", "", "", "b"]
In addition using Splitter makes the code more readable as Splitter has
nicer methods that make it clearer which high-level operation should be
performed. E.g. in some places we can use
Splitter.on(CharMatcher.anyOf(something)) instead of matching on
patterns.
Tests and classes that are used by the GWT UI are not adapted.
[1] http://konigsberg.blogspot.com/2009/11/final-thoughts-java-puzzler-splitting.html
Change-Id: I6f141fb492e2fb94544089d794f245d0885f8649
Signed-off-by: Edwin Kempin <ekempin@google.com>
If the values are unexpected we don't fail but ignore the values. It's
sufficient to log this as a warning.
Change-Id: I5eb843f469a2bc911c699b748388c0673df54c11
Signed-off-by: Edwin Kempin <ekempin@google.com>
Without the change ID one doesn't know for which change the reviewer
fields have unexpected values.
Change-Id: I3003acc4cd0f54c13fce4be67d6e092c1dd67412
Signed-off-by: Edwin Kempin <ekempin@google.com>
Since ExternalId.USER_NAME_PATTERN_REGEX is now no longer used by
external callers it is made private.
Change-Id: Id7a108bf39baffc52273879bbf8973dbbea0ae0c
Signed-off-by: Edwin Kempin <ekempin@google.com>
The testing package has its own bazel package (it has BUILD file) and
this is not part of the final artifact built with common:client rule.
However, because of wild card inclusion in Common.gwt.xml, the whole
tree is included in GWT module, when used from Eclipse in super dev mode
debug sessions. To avoid needing to add @GwtIncompatible to all files in
this package, exclude this package from GWT module source path in the GWT
module definition.
Change-Id: Ib36348084d59cc5e10f8dc5c7c627fc4e3323609
The method GroupControl.Factory#controlFor(AccountGroup.Id) was only
used by GroupsCollection#parseId. When this call is inlined, it becomes
obvious that the creation of a GroupControl instance is unnecessary.
This change has the additional benefit of reducing the use of
AccountGroup.Id in our code base and to keep new code from using
GroupControl.Factory#controlFor(AccountGroup.Id) instead of the
recommended method GroupControl.Factory#controlFor(AccountGroup.UUID).
Change-Id: I9c3b69769a567192ec4582e11bd774c942e1b068
Most of Gerrit's core code switched to use AccountGroup.UUID instead of
AccountGroup.Id to refer to a specific group. In theory when we are on
NoteDb for groups, we should only need to support the lookup of a group
by AccountGroup.Id for resource lookups of the REST API.
The lookup by AccountGroup.Id which was present in CreateGroup wasn't
really necessary. It was a relic from former times and probably wasn't
changed up to now as CreateGroupArgs is used as part of the extension
point GroupCreationValidationListener. Considering that we already broke
other extension points relating to groups with this release (as e.g. in
I6d7d97e9), it's better that we make this change now instead of later.
Change-Id: I2b7eb7f1596a1c1147bf552990b793564ba850e6
The built-in log formatter uses curly braces instead of '%s'. Without
this change, log messages look like:
ERROR com.google.gerrit.server.index.change.ChangeField : Invalid
value for reviewer field: %s
Change-Id: Ia93019c3591528771c88a735067a0ae6ac83cc40
* stable-2.15:
Bazel: Silent zip output in gerrit-antlr:query_antlr rule
Provide mvn command output when VERBOSE set
ReviewerRecommender: Prevent NPE when changeNotes is null
ReviewerSuggestion: More Javadoc improvements
Change-Id: I433e257961e14b38332997daea8c6e39c42b7d98
* changes:
RestApiServlet: Merge if with enclosing one
RestApiServlet: Remove redundant else statements
RestApiServlet: Use log built-in string formatting
RestApiServlet: Use defined method to check content type
RestApiServlet: Extract constant
RestApiServlet: Extract methods to find kind of request
RestApiServlet: Collapse identically handled exceptions
* stable-2.15:
Document that NoteDb migration requires a large heap
Move downloaded artifact cache from buck-cache to bazel-cache
Move downloaded artifact cache from buck-cache to bazel-cache
Revert "Hide sensitive data from audit and gerrit logs"
dev-plugins: Improve formatting of reviewer suggestion documentation
ReviewerSuggestion: Reword Javadoc
ReviewerRecommender: Add debug log of plugin provided weight
Handle deleted project in ReindexIfStaleTask
Disallow tabbing on paper-button
Handle the ReindexIfStale event when a change is deleted
PolyGerrit: Fix gr-diff-view arrows to use html code
Migrate metrics-core to 4.0.2 version
CreateChange: Fix appending Signed-off-by line after Change-Id
CreateChange: Only insert Change-Id if there isn't already one
CreateChangeIT: Disable "Insert Signed-off-by" after test
Fix gr-group-members to add and delete using group id rather than name
Fix group member URL
Change-Id: I5ccbc4265266236b55b3c864347e7016cc7a2bd0
Most references are in documentation and comments. The main
developer-visible behavior change is moving the downloaded artifact
cache from ~/.gerritcodereview/buck-cache to bazel-cache, which will
result in re-downloading dependencies on the next build; this had to
happen sooner or later. Alternatives, which are not worth the effort,
include teaching the scripts to accept both locations, or having it
rearrange and/or symlink directories behind the scenes.
There are just a few references remaining, all of which are intentional:
$ git grep -Pi '\bbuck(lets?)?\b' HEAD
HEAD:java/com/google/gerrit/httpd/raw/StaticModule.java: // https://gerrit-review.googlesource.com/#/c/57570/57/gerrit-httpd/BUCK@32
HEAD:resources/com/google/gerrit/server/mime/mime-types.properties:bucklet = text/x-python
HEAD:resources/com/google/gerrit/server/mime/mime-types.properties:BUCK = text/x-python
Change-Id: Idb93a483451ccf86ba96c379d38008a7894c3f95
