Since version 4.13 JUnit has an assertThrows method. Remove the
implementation in MoreAsserts and use the one from JUnit.
CQ: 21439
Change-Id: I086baa94aa3069cebe87c4cbf91ed1534523c6cb
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
and switch over strings where possible. Sometimes if statements are
chained and form a series of comparisons against constants. Using switch
statements improves readability.
Bug: 545856
Change-Id: Iacb78956ee5c20db4d793e6b668508ec67466606
Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Add the following statistics
- cache hit count and hit ratio
- cache miss count and miss ratio
- count of successful and failed loads
- rate of failed loads
- load, eviction and request count
- average and total load time
Use LongAdder instead of AtomicLong to implement counters in order to
improve scalability.
Optionally expose these metrics via JMX, they are registered with the
platform MBean server if the config option jmx.WindowCacheStats = true
in the user or system level git config.
Bug: 553573
Change-Id: Ia2d5246ef69b9c2bd594a23934424bc5800774aa
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
ReftableTest: Clean up boxing warnings on usage of String.format
Passing int as an argument to String.format causes a warning:
The expression of type int is boxed into Integer
Most of these are already suppressed, but there are a couple that are
not. Add suppressions for those.
For the existing ones, move the suppression scope from the method to
the actual usage. Where necessary extract the usage out to a local
variable.
Change-Id: I7a7ff6dec49467e4b5c58d27a231c74e6e1c5437
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
The reftable format supports fast inverse (SHA1 => ref) queries.
If the ref database does not support fast inverse queries, it may be
advantageous to build a complete SHA1 to ref map in advance for
multiple uses. To let applications decide, this function indicates
whether the inverse map is available.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: Idaf7e01075906972ec21332cade285289619c2b3
RepositoryCache: don't require HEAD in git repositories
Reftable-enabled repositories don't have a file called HEAD. Check for
reftable/ instead.
This fixes repository creation on reftable in Gerrit.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: I778c2be01d96aaf135affae4b457b5fe5b483bee
Reftable is a binary, block-based storage format for the ref-database.
It provides several advantages over the traditional packed + loose
storage format:
* O(1) write performance, even for deletions and transactions.
* atomic updates to the ref database.
* O(log N) lookup and prefix scans
* free from restrictions imposed by the file system: it is
case-sensitive even on case-insensitive file systems, and has
no inherent limitations for directory/file conflicts
* prefix compression reduces space usage for repetitive ref names,
such as gerrit's refs/changes/xx/xxxxx format.
FileReftableDatabase is based on FileReftableStack, which does
compactions inline. This is simple, and has good median performance,
but every so often it will rewrite the entire ref database.
For testing, a FileReftableTest (mirroring RefUpdateTest) is added to
check for Reftable specific behavior. This must be done separately, as
reflogs have different semantics.
Add a reftable flavor of BatchRefUpdateTest.
Add a FileReftableStackTest to exercise compaction.
Add FileRepository#convertToReftable so existing testdata can be
reused.
CQ: 21007
Change-Id: I1837f268e91c6b446cb0155061727dbaccb714b8
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
reftable: enforce ascending order in sortAndWriteRefs
MergedReftableTest#scanDuplicates tests whether we can write duplicate
keys in a merged reftable. Apparently, the first key appearing should
get precedence, and this works because the sort() algorithm on ordered
collections is stable.
This is potentially confusing behavior, because you can write data
into the table that cannot be retrieved (Merged table can only have
one entry per key), and the APIs such as exactRef() only return a
single value.
Make this consistent with behavior introduced in I04f55c481 "reftable:
enforce ordering for ref and log writes" by considering a duplicate key
in sortAndWriteRefs as a fatal runtime error.
Change-Id: I1eedd18f028180069f78c5c467169dcfe1521157
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
This doesn't yet ensure that _all_ repositories are closed. It only
handles the obvious, local, and easy cases.
Change-Id: I0f9f8607791f0f03ed1f5ad71e9595e78b78892f
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Enable and fix "Statement unnecessarily nested within else clause" warnings
Since [1] the gerrit project includes jgit as a submodule, and has this
warning enabled, resulting in 100s of warnings in the console.
Also enable the warning here, and fix them.
At the same time, add missing braces around adjacent and nearby one-line
blocks.
[1] https://gerrit-review.googlesource.com/c/gerrit/+/227897
Change-Id: I81df3fc7ed6eedf6874ce1a3bedfa727a1897e4c
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
On changing a ref, the old SHA1 is not updated in the object => ref
mapping. This means search by object ID may still turn up a ref from
deeper within the stack. To fix this, check all refs produced by the
merged iterator against the merged reftables.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: I41e9cd395b0608eedeeaead0a9fd997238d747c9
MergedReftable is not used as an AutoCloseable, because closing tables
is currently handled by DfsReftableStack#close.
Encode that a MergedReftable is a list of ReftableReaders. The previous
code suggested that we could form nested trees of MergedReftables,
which is not how we use reftables.
Change-Id: Icbe2fee8a5a12373f45fc5f97d8b1a2b14231c96
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
This makes the intended use of the classes more clear. It also
simplifies generic functions that write reftables: they only need a
ReftableWriter as argument, as the stream is carried within the
ReftableWriter.
Change-Id: Idbb06f89ae33100f0c0b562cc38e5b3b026d5181
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
reftable: read file footer in ReftableReader#allRefs
allRefs determined the end of the ref block without accounting for
index or log blocks. This could cause other blocks to be interpreted
as ref blocks, leading to "invalid block" error messages.
Change-Id: I7b9323e7d5e0e7d64535b3ec1efd576aed1e9870
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
BatchRefUpdate: repro racy atomic update, and fix it
PackedBatchRefUpdate was creating a new packed-refs list that was
potentially unsorted. This would be papered over when the list was
read back from disk in parsePackedRef, which detects unsorted ref
lists on reading, and sorts them. However, the BatchRefUpdate also
installed the new (unsorted) list in-memory in
RefDirectory#packedRefs.
With the timestamp granularity code committed to stable-5.1, we can
more often accurately decide that the packed-refs file is clean, and
will return the erroneous unsorted data more often. Unluckily timed
delays also cause the file to be clean, hence this problem was
exacerbated under load.
The symptom is that refs added by a BatchRefUpdate would stop being
visible directly after they were added. In particular, the Gerrit
integration tests uses BatchRefUpdate in its setup for creating the
Admin group, and then tries to read it out directly afterward.
The tests recreates one failure case. A better approach would be to
revise RefList.Builder, so it detects out-of-order lists and
automatically sorts them.
Fixes https://bugs.eclipse.org/bugs/show_bug.cgi?id=548716 and
https://bugs.chromium.org/p/gerrit/issues/detail?id=11373.
Bug: 548716
Change-Id: I613c8059964513ce2370543620725b540b3cb6d1
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Previously, the API did not enforce ordering of writes. Misuse of
this API would lead to data effectively being lost.
Guard against that with IllegalArgumentException, and add a test.
Change-Id: I04f55c481d60532fc64d35fa32c47037a03988ae
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
reftable: fix seeking to refs in reflog implementation
Small reftables omit the log index. Currently,
ReftableWriter#shouldHaveIndex does this if there is a single-block
log, but other writers could decide on different criteria.
In the case that the log index is missing, we have to linearly search
for the right block. It is never appropriate to use binary search on
blocks for log data, as the blocks are compressed and therefore
irregularly sized.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: Id59874edf6bf45c7dec502d9465888e077ffe198
LocalDiskRefTreeDatabaseTest shall use MockSystemReader
It missed to call the setup() method of its super class which prepares
the MockSystemReader
Change-Id: I39858749f8d0115fc6ac7edc8847ffb2bbc85c33
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
If we use the default system reader FileStoreAttributes cannot persist
attributes in userConfig when tests run in Bazel due to sandboxing.
Hence we need to ensure that all tests use MockSystemReader (and
especially a mocked userConfig).
Change-Id: Ic1ad8e2ec5a150c5433434a5f6667d6c4674c87d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[error prone] suppress AmbiguousMethodReference in AnyObjectId
Move the implementation of the static equals() method to a new method
and suppress the error. Deprecate the old method to signal that we
intend to remove it in the next major release.
See https://errorprone.info/bugpattern/AmbiguousMethodReference
Change-Id: I5e29c97f4db3e11770be589a6ccd785e2c9ac7f2
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Increase the safety factor to 2.5x for extra safety if max of measured
timestamp resolution and measured minimal racy threshold is < 100ms, use
1.25 otherwise since for large filesystem resolution values the
influence of finite resolution of the system clock should be negligible.
Before, not yet using the newly introduced minRacyThreshold measurement,
the threshold was 1.1x FS resolution, and we could issue the
following sequence of events,
start
create-file
read-file (currentTime)
end
which had the following timestamps:
create-file 1564589081
start 1564589082
read 1564589082
end 1564589082
In this case, the difference between create-file and read is 5ms,
which exceeded the 4ms FS resolution, even though the events together
took just 2ms of runtime.
Reproduce with:
bazel test --runs_per_test=100 \
//org.eclipse.jgit.test:org_eclipse_jgit_internal_storage_file_FileSnapshotTest
The file system timestamp resolution is 4ms in this case.
This code assumes that the kernel and the JVM use the same clock that
is synchronized with the file system clock. This seems plausible,
given the resolution of System.currentTimeMillis() and the latency for
a gettimeofday system call (typically ~1us), but it would be good to
justify this with specifications.
Also cover a source of flakiness: if the test runs under extreme load,
then we could have
start
create-file
<long delay>
read
end
which would register as an unmodified file. Avoid this by skipping the
test if end-start is too big.
[msohn]:
- downported from master to stable-5.1
- skip test if resolution is below 10ms
- adjust safety factor to 1.25 for resolutions above 100ms
Change-Id: I87d2cf035e01c44b7ba8364c410a860aa8e312ef
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Persist minimal racy threshold and allow manual configuration
To enable persisting the minimal racy threshold per FileStore add a
new config option to the user global git configuration:
- Config section is "filesystem"
- Config subsection is concatenation of
- Java vendor (system property "java.vendor")
- Java version (system property "java.version")
- FileStore's name, on Windows we use the attribute volume:vsn instead
since the name is not necessarily unique.
- separated by '|'
e.g.
"AdoptOpenJDK|1.8.0_212-b03|/dev/disk1s1"
The same prefix is used as for filesystem timestamp resolution, so
both values are stored in the same config section
- The config key for minmal racy threshold is "minRacyThreshold" as a
time value, supported time units are those supported by
DefaultTypedConfigGetter#getTimeUnit
- measure for 3 seconds to limit runtime which depends on hardware, OS
and Java version being used
If the minimal racy threshold is configured for a given FileStore the
configured value is used instead of measuring it.
When the minimal racy threshold was measured it is persisted in the user
global git configuration.
Rename FileStoreAttributeCache to FileStoreAttributes since this class
is now declared public in order to enable exposing all attributes in one
object.
Example:
[filesystem "AdoptOpenJDK|11.0.3|/dev/disk1s1"]
timestampResolution = 7000 nanoseconds
minRacyThreshold = 3440 microseconds
Change-Id: I22195e488453aae8d011b0a8e3276fe3d99deaea
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Also-By: Marc Strapetz <marc.strapetz@syntevo.com>
Measure minimum racy interval to auto-configure FileSnapshot
By running FileSnapshotTest#detectFileModified we found that the sum of
measured filesystem timestamp resolution and measured clock resolution
may yield a too small interval after a file has been modified which we
need to consider racily clean. In our tests we didn't find this behavior
on all systems we tested on, e.g. on MacOS using APFS and Java 8 and 11
this effect was not observed.
On Linux (SLES 15, kernel 4.12.14-150.22-default) we collected the
following test results using Java 8 and 11:
In 23-98% of 10000 test runs (depending on filesystem type and Java
version) the test failed, which means the effective interval which needs
to be considered racily clean after a file was modified is larger than
the measured file timestamp resolution.
"delta" is the observed interval after a file has been modified but
FileSnapshot did not yet detect the modification:
"resolution" is the measured sum of file timestamp resolution and clock
resolution seen in Java.
Java version filesystem failures resolution min delta max delta
1.8.0_212-b04 btrfs 98.6% 1 ms 3.6 ms 6.6 ms
1.8.0_212-b04 ext4 82.6% 3 ms 1.1 ms 4.1 ms
1.8.0_212-b04 xfs 23.8% 4 ms 3.7 ms 3.9 ms
1.8.0_212-b04 zfs 23.1% 3 ms 4.8 ms 5.0 ms
11.0.3+7 btrfs 98.1% 3 us 0.7 ms 4.7 ms
11.0.3+7 ext4 98.1% 6 us 0.7 ms 4.7 ms
11.0.3+7 xfs 98.5% 7 us 0.1 ms 8.0 ms
11.0.3+7 zfs 98.4% 7 us 0.7 ms 5.2 ms
Mac OS
1.8.0_212 APFS 0% 1 s
11.0.3+7 APFS 0% 6 us
The observed delta is not distributed according to a normal gaussian
distribution but rather random in the observed range between "min delta"
and "max delta".
Run this test after measuring file timestamp resolution in
FS.FileAttributeCache to auto-configure JGit since it's unclear what
mechanism is causing this effect.
In FileSnapshot#isRacyClean use the maximum of the measured timestamp
resolution and the measured "delta" as explained above to decide if a
given FileSnapshot is to be considered racily clean. Add a 30% safety
margin to ensure we are on the safe side.
Change-Id: I1c8bb59f6486f174b7bbdc63072777ddbe06694d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
These are useful to avoid typos, and also for tab completion.
Change-Id: I0f2d267e46b36bc40297c9657c447f3fd8b9f831
Signed-off-by: David Turner <dturner@twosigma.com>
Repeat the test 10000 times to get statistics if measured
fsTimestampResolution is working in practice to detect racy git
situations.
Add a class to compute statistics for this test. Log delta between
lastModified and time when FileSnapshot failed to detect modification.
This happens if the racy git limit determined by measuring filesystem
timestamp resolution and clock resolution is too small. If it would be
correct FileSnapshot would always detect modification or mark it
modified if time since modification is smaller than the racy git limit.
Change-Id: Iabe7af1a7211ca58480f8902d4fa4e366932fc77
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Repeat RefDirectoryTest.testGetRef_DiscoversModifiedLoose 100 times
This should help to detect if measured fsTimeResolution is too small.
Change-Id: Id1f54dbdedb52b17859904e47776fa3a5887b8be
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Fix FileSnapshotTests for filesystem with high timestamp resolution
When filesystem timestamp resolution is very high some tests don't work
since runtime of the test setup is too long to reach a racily clean
FileSnapshot. Hence skip these tests when timestamp resolution is higher
than 10 millisecond.
Change-Id: Ie47dd10eda22037b5c1ebff6b6becce0654ea807
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Measure filesystem timestamp resolution already in test setup
This helps to avoid some time critical tests can't prepare the test
fixture intended since measuring timestamp resolution takes time.
Change-Id: Ib34023e682a106070ca97e98ef16789a4dfb97b4
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
- use Path instead of File
- create test directories, files and output stream using Files methods
- delete unused list "files"
Change-Id: I8c5c601eca9f613efb5618d33b262277df92a06a
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Use Instant instead of milliseconds for filesystem timestamp handling
This enables higher file timestamp resolution on filesystems like ext4,
Mac APFS (1ns) or NTFS (100ns) providing high timestamp resolution on
filesystem level.
Note:
- on some OSes Java 8,9 truncate milliseconds, see
https://bugs.openjdk.java.net/browse/JDK-8177809, fixed in Java 10
- UnixFileAttributes truncates timestamp resolution to microseconds when
converting the internal representation to FileTime exposed in the API,
see https://bugs.openjdk.java.net/browse/JDK-8181493
- WindowsFileAttributes also provides only microsecond resolution
Change-Id: I25ffff31a3c6f725fc345d4ddc2f26da3b88f6f2
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Add a unittest.
In commit I5485db55 ("Fix FileSnapshot's consideration of file size"),
the special casing of UNKNOWN_SIZE was forgotten.
This change, together with I493f3b57b ("Measure file timestamp
resolution used in FileSnapshot") introduced a regression that would
occasionally surface in Gerrit integration tests marked UseLocalDisk,
with the symptom that creating the Admin user in NoteDb failed with a
LOCK_FAILURE.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: I7ffd972581f815c144f810481103c7985af5feb0
As reported by Error Prone:
An inner class should be static unless it references members of its
enclosing class. An inner class that is made non-static unnecessarily
uses more memory and does not make the intent of the class clear.
See https://errorprone.info/bugpattern/ClassCanBeStatic
Change-Id: Ib99d120532630dba63cf400cc1c61c318286fc41
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
(cherry picked from commit ee40efcea4)
ErrorProne: Increase severity of FutureReturnValueIgnored to ERROR
The only remaining code where the return value is ignored is in tests.
Update them to store the value and perform a basic assertion.
Change-Id: I29ef5bd5dd0648aac3490f9e47ecc74544109652
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Test that JGit detects that packfiles have changed even if they are
repacked multiple times in one tick of the filesystem timer.
Test that this detection works also when repacking doesn't change the
length or the filekey of the packfile. In this case where a modified
file can't be detected by looking at file metadata JGit should still
detect too fast modification by racy git checks and trigger rescanning
the pack list and consequently rereading of packfile content.
Change-Id: I67682cfb807c58afc6de9375224ff7489d6618fb
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Extend FileSnapshot for packfiles to also use checksum to detect changes
If the attributes of FileSnapshot don't detect modification of a
packfile read the packfile's checksum and compare it against the
checksum cached in the loaded packfile.
Since reading the checksum needs less IO than reloading the complete
packfile this may help to reduce the overhead to detect modficiation
when a gc completes while ObjectDirectory scans for packfiles in another
thread.
Bug: 546891
Change-Id: I9811b497eb11b8a85ae689081dc5d949ca8c4be5
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Capture reason for result of FileSnapshot#isModified
This allows to verify the expected behavior in
FileSnapshotTest#testSimulatePackfileReplacement and enables extending
FileSnapshot for packfiles to read the packfile's checksum as another
criterion to detect modifications without reading the full content.
Also add another field capturing the result of the last check if
lastModified was racily clean.
Remove unnecessary determination of raciness in the constructor. It was
determined twice in all relevant cases.
Change-Id: I100a2f49d7949693d7b72daa89437e166f1dc107
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Skip FileSnapshotTest#testSimulatePackfileReplacement on Windows
NTFS does not support FileKey hence ignore this test on Windows.
Change-Id: I7b53a591daa5e03eb5e401b5b26d612ab68ce10d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Fix FileSnapshotTest.testNewFileNoWait() to match its javadoc
testNewFileNoWait() was identical to testNewFileWithWait() but claims it
doesn't wait at all. Hence remove the waits.
Change-Id: I49b8ca5cb49a43c55fe61870c18c42f32fb4b74d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Matthias Sohn
David Pursehouse
Carsten Hammer
Han-Wen Nienhuys
Thomas Wolf
David Turner
Christian Halstrick