Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Th2-5165] Use cache in BookInfo #253

Merged
merged 38 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
c360d21
[TH2-5165] Use cache in BookInfo
Nikita-Smirnov-Exactpro Feb 15, 2024
19638d1
[TH2-5165] Removed old BookInfo constructor
Nikita-Smirnov-Exactpro Feb 19, 2024
66beb2a
[TH2-5165] Corrected according integration tests
Nikita-Smirnov-Exactpro Feb 19, 2024
10c1056
[TH2-5165] bookId, name, start are unique page identifier
Nikita-Smirnov-Exactpro Feb 19, 2024
ad5f947
[TH2-5165] corrected after review
Nikita-Smirnov-Exactpro Feb 19, 2024
4414d01
[TH2-5165] solved todo/fixme
Nikita-Smirnov-Exactpro Feb 19, 2024
44c5582
[TH2-5165] Added book info metrics
Nikita-Smirnov-Exactpro Feb 20, 2024
9191fd2
[TH2-5165] Added Iterator<PageInfo> getPages
Nikita-Smirnov-Exactpro Feb 20, 2024
a8e2c8f
[TH2-5165] corrected after review
Nikita-Smirnov-Exactpro Feb 21, 2024
2f6c263
[TH2-5165] migrated to IteratorPa
Nikita-Smirnov-Exactpro Feb 22, 2024
d50305e
[TH2-5165] refactored after migration
Nikita-Smirnov-Exactpro Feb 22, 2024
2f298c9
[TH2-5165] Revert API methods
Nikita-Smirnov-Exactpro Feb 22, 2024
6ec65e9
[TH2-5165] Added CradleStorage.getBook method
Nikita-Smirnov-Exactpro Feb 23, 2024
fc0c477
[TH2-5165] getPages(start, end, order) can handle start > end case
Nikita-Smirnov-Exactpro Feb 26, 2024
3a6e9be
[TH2-5165] Removed ThreadSafeProvider class
Nikita-Smirnov-Exactpro Feb 26, 2024
b1d9114
[TH2-5165] Corrected java doc in BookInfo
Nikita-Smirnov-Exactpro Feb 26, 2024
8852126
[TH2-5165] Corrected after review
Nikita-Smirnov-Exactpro Feb 26, 2024
1a848c6
[TH2-5165] Corrected after review
Nikita-Smirnov-Exactpro Feb 26, 2024
fc892ce
[TH2-5165] Fixed problem with Instant MIN/MAX conversion problem
Nikita-Smirnov-Exactpro Feb 27, 2024
2262510
[TH2-5165] Corrected book cache behaviour when book has gap
Nikita-Smirnov-Exactpro Feb 28, 2024
9021c09
[TH2-5165] Added testAddPageInTheMiddleOfExist
Nikita-Smirnov-Exactpro Feb 29, 2024
121d11c
[TH2-5165] corrected after review
Nikita-Smirnov-Exactpro Mar 1, 2024
cb0352e
Update cradle-cassandra/src/test/java/com/exactpro/cradle/cassandra/i…
Nikita-Smirnov-Exactpro Mar 1, 2024
6a5a1ad
[TH2-5165] corrected after review
Nikita-Smirnov-Exactpro Mar 1, 2024
77e8734
[TH2-5165] removed java 17 API
Nikita-Smirnov-Exactpro Mar 1, 2024
ebee034
[TH2-5165] corrected book info metrics
Nikita-Smirnov-Exactpro Mar 1, 2024
4ce8946
[TH2-5165] added testAddTheSecondPageBeforeThreshold test
Nikita-Smirnov-Exactpro Mar 4, 2024
4e0ecca
[TH2-5165] added randomAccessDaysInvalidateInterval option
Nikita-Smirnov-Exactpro Mar 4, 2024
c6e2979
[TH2-5165] corrected after review
Nikita-Smirnov-Exactpro Mar 4, 2024
7ecc0ed
[TH2-5165] revert BOOK_LABEL to book
Nikita-Smirnov-Exactpro Mar 5, 2024
0d29959
[TH2-5165] revert logic to skip register load for empty day page inte…
Nikita-Smirnov-Exactpro Mar 5, 2024
b88f91e
[TH2-5165] revert logic to skip register load for empty day page inte…
Nikita-Smirnov-Exactpro Mar 5, 2024
e9b15b6
Merge remote-tracking branch 'origin/dev-version-5' into TH2-5165
Nikita-Smirnov-Exactpro Mar 6, 2024
7f2926b
[TH2-5165] merged with dev-version-5
Nikita-Smirnov-Exactpro Mar 6, 2024
a578802
[TH2-5165] added BookWithoutPageTest
Nikita-Smirnov-Exactpro Mar 6, 2024
31756d3
[TH2-5165] corrected after review
Nikita-Smirnov-Exactpro Mar 6, 2024
ac5ab52
[TH2-5165] Updated github workflow
Nikita-Smirnov-Exactpro Mar 6, 2024
afe632e
[TH2-5165] Updated github workflow
Nikita-Smirnov-Exactpro Mar 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -195,16 +195,17 @@ Test events have mandatory parameters that are verified when storing an event. T

## Metrics

* cradle_page_cache_page_request_total (type: Counter, labels: book, cache, method) - Page requests number from cache
* cache: HOT, RANDOM
* method: GET, NEXT, PREVIOUS, FIND
* cradle_page_cache_invalidate_total (type: Counter, labels: book, cache, cause) - Cache invalidates
* `cradle_page_cache_size` (type: Gauge, labels: book, cache) - Size of page cache.
* `cradle_page_cache_page_request_total` (type: Counter, labels: book, cache, method) - Page requests number from cache
* `cradle_page_cache_invalidate_total` (type: Counter, labels: book, cache, cause) - Cache invalidates
* `cradle_page_cache_page_loads_total` (type: Summary, labels: book, cache) - Page loads number to cache
* `_count` - loaded page day intervals
* `_sum` - loaded pages

### Labels:
* cache: HOT, RANDOM
* method: GET, NEXT, PREVIOUS, FIND, ITERATE, REFRESH
* cause: EXPLICIT, REPLACED, COLLECTED, EXPIRED, SIZE
* cradle_page_cache_page_loads_total (type: Summary, labels: book, cache) - Page loads number to cache
*_count - loaded page day intervals
*_sum - loaded pages
* cache: HOT, RANDOM

## Release notes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public abstract class BaseCradleCassandraTest {

/*
Following method should be used in beforeClass if extending class
wants to implement it's own logic of initializing books and pages
wants to implement its own logic of initializing books and pages
*/
protected void startUp() throws IOException, InterruptedException, CradleStorageException {
startUp(false);
Expand Down Expand Up @@ -168,7 +168,7 @@ protected TestEventToStore generateTestEvent (String scope, Instant start, long
@NotNull
protected static Map<PageId, PageInfo> toMap(Collection<PageInfo> result) {
return result.stream()
.collect(Collectors.toMap(
.collect(Collectors.toUnmodifiableMap(
PageInfo::getId,
Function.identity()
));
Expand Down
65 changes: 29 additions & 36 deletions cradle-core/src/main/java/com/exactpro/cradle/BookInfo.java
OptimumCode marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@
import static com.exactpro.cradle.BookInfoMetrics.CacheName.RANDOM;
import static com.exactpro.cradle.BookInfoMetrics.RequestMethod.FIND;
import static com.exactpro.cradle.BookInfoMetrics.RequestMethod.GET;
import static com.exactpro.cradle.BookInfoMetrics.RequestMethod.ITERATE;
import static com.exactpro.cradle.BookInfoMetrics.RequestMethod.NEXT;
import static com.exactpro.cradle.BookInfoMetrics.RequestMethod.PREVIOUS;
import static com.exactpro.cradle.BookInfoMetrics.RequestMethod.REFRESH;
import static java.util.Collections.emptyIterator;
import static java.util.Collections.unmodifiableMap;
import static java.util.Collections.unmodifiableNavigableMap;
Expand All @@ -61,6 +63,11 @@ public class BookInfo {
private static final int MILLISECONDS_IN_DAY = SECONDS_IN_DAY * 1_000;
private static final long MAX_EPOCH_DAY = getEpochDay(Instant.MAX);
private static final IPageInterval EMPTY_PAGE_INTERVAL = new EmptyPageInterval();

static {
METRICS.setPageCacheSize("", HOT, HOT_CACHE_SIZE);
}

private final BookId id;
private final String fullName,
desc;
Expand Down Expand Up @@ -94,24 +101,14 @@ public BookInfo(BookId id,
this.hotCache = Caffeine.newBuilder()
.maximumSize(HOT_CACHE_SIZE)
.removalListener((epochDay, pageInterval, cause) -> METRICS.incInvalidate(id.getName(), HOT, cause))
.build(epochDay -> {
IPageInterval pageInterval = createPageInterval(HOT, epochDay);
if (pageInterval != null) {
METRICS.incLoads(id.getName(), pageInterval.getCacheName(), pageInterval.size());
}
return pageInterval;
});
.build(epochDay -> createPageInterval(HOT, epochDay));

this.randomAccessCache = Caffeine.newBuilder()
.maximumSize(cacheSize)
.removalListener((epochDay, pageInterval, cause) -> METRICS.incInvalidate(id.getName(), RANDOM, cause))
.build(epochDay -> {
IPageInterval pageInterval = createPageInterval(RANDOM, epochDay);
if (pageInterval != null) {
METRICS.incLoads(id.getName(), pageInterval.getCacheName(), pageInterval.size());
}
return pageInterval;
});
.build(epochDay -> createPageInterval(RANDOM, epochDay));

METRICS.setPageCacheSize(id.getName(), RANDOM, cacheSize);
}

public BookId getId() {
Expand Down Expand Up @@ -159,7 +156,6 @@ public Collection<PageInfo> getPages() {

public PageInfo getPage(PageId pageId) {
IPageInterval pageInterval = getPageInterval(pageId.getStart());
METRICS.incRequest(id.getName(), pageInterval.getCacheName(), GET);
return pageInterval.get(pageId);
}

Expand Down Expand Up @@ -237,7 +233,6 @@ public PageInfo findPage(Instant timestamp) {
// Search in the page interval related to the timestamp
IPageInterval pageInterval = getPageInterval(epochDay);
PageInfo pageInfo = pageInterval.find(timestamp);
METRICS.incRequest(id.getName(), pageInterval.getCacheName(), FIND);
if (pageInfo != null) {
return pageInfo;
}
Expand Down Expand Up @@ -272,10 +267,8 @@ public PageInfo findPage(Instant timestamp) {
// page interval without new pages
continue;
}
PageInfo previousPageInfo = previousPageInterval.find(timestamp);
METRICS.incRequest(id.getName(), previousPageInterval.getCacheName(), FIND);
// sP1 ... | ... eP1 ^ ... gap ... sP2 ... |
return previousPageInfo;
return previousPageInterval.find(timestamp);
}
throw new IllegalStateException(
"First page is before requested timestamp, but neither cache contains appropriate page, requested: " +
Expand All @@ -290,26 +283,22 @@ public PageInfo findPage(Instant timestamp) {
public PageInfo getNextPage(Instant startTimestamp) {
long epochDate = getEpochDay(startTimestamp);
IPageInterval pageInterval = getPageInterval(epochDate);
METRICS.incRequest(id.getName(), pageInterval.getCacheName(), NEXT);
PageInfo currentInterval = pageInterval.next(startTimestamp);
if (currentInterval != null) {
return currentInterval;
}
pageInterval = getPageInterval(epochDate + 1);
METRICS.incRequest(id.getName(), pageInterval.getCacheName(), NEXT);
return pageInterval.next(startTimestamp);
}

public PageInfo getPreviousPage(Instant startTimestamp) {
long epochDate = getEpochDay(startTimestamp);
IPageInterval pageInterval = getPageInterval(epochDate);
METRICS.incRequest(id.getName(), pageInterval.getCacheName(), PREVIOUS);
PageInfo currentInterval = pageInterval.previous(startTimestamp);
if (currentInterval != null) {
return currentInterval;
}
pageInterval = getPageInterval(epochDate - 1);
METRICS.incRequest(id.getName(), pageInterval.getCacheName(), PREVIOUS);
return pageInterval.previous(startTimestamp);
}

Expand Down Expand Up @@ -342,7 +331,9 @@ public String toString() {
*/
void refresh() {
firstPage.set(null);
METRICS.incRequest(id.getName(), HOT, REFRESH);
hotCache.invalidateAll();
METRICS.incRequest(id.getName(), RANDOM, REFRESH);
randomAccessCache.invalidateAll();

getFirstPage();
Expand Down Expand Up @@ -403,7 +394,12 @@ private IPageInterval getPageInterval(Instant timestamp) {
Instant start = toInstant(epochDay);
Instant end = start.plus(1, ChronoUnit.DAYS).minus(1, ChronoUnit.NANOS);
Collection<PageInfo> loaded = pagesLoader.load(id, start, end);
return loaded.isEmpty() ? null : create(id, start, cacheName, loaded);
if (loaded.isEmpty()) {
return null;
}
// We shouldn't register metric to avoid the `loads` >> `requests` case
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please advise what this means exactly? I am looking at the code and I have no clue how to interpret this sentence

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this comment when work on the dashboard for hit / miss rates. But I think I doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that it is hard to understand what this comment tries to say) Even after your comment here I still cannot get it... I think if we leave something in the code it should be clear what it says. More than one line can be used to express what you mean.
This concerns me because if the task is assigned to somebody else he or she needs all the information to continue the work. And our responsibility to leave this information either in the code or in the issue. And this information should not require decryption)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood you point and try to solve short comment in future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The origin comment should be
We shouldn't register metric for empty page intervals to avoid the loads >> requests case
When loads number more than requests number, current dashboard for calculating hit / miss rate shows wrong values

METRICS.incLoads(id.getName(), cacheName, loaded.size());
return create(id, start, cacheName, loaded);
}

private static long currentEpochDay() {
Expand All @@ -419,8 +415,6 @@ private static Instant toInstant(long epochDay) {
}

private interface IPageInterval {
BookInfoMetrics.CacheName getCacheName();

int size();

PageInfo get(PageId pageId);
Expand All @@ -435,10 +429,6 @@ private interface IPageInterval {
}

private static final class EmptyPageInterval implements IPageInterval {
@Override
public BookInfoMetrics.CacheName getCacheName() {
return null;
}

@Override
public int size() {
Expand Down Expand Up @@ -472,32 +462,32 @@ public Stream<PageInfo> stream(Instant leftBoundTimestamp, Instant rightBoundTim
}

private static class PageInterval implements IPageInterval {
private final BookId bookId;
private final BookInfoMetrics.CacheName cacheName;
private final Map<PageId, PageInfo> pageById;
private final NavigableMap<Instant, PageInfo> pageByInstant;

private PageInterval(BookInfoMetrics.CacheName cacheName, Map<PageId, PageInfo> pageById, NavigableMap<Instant, PageInfo> pageByInstant) {
private PageInterval(BookId bookId, BookInfoMetrics.CacheName cacheName, Map<PageId, PageInfo> pageById, NavigableMap<Instant, PageInfo> pageByInstant) {
this.bookId = bookId;
this.cacheName = cacheName;
this.pageById = pageById;
this.pageByInstant = pageByInstant;
}

public BookInfoMetrics.CacheName getCacheName() {
return cacheName;
}

@Override
public int size() {
return pageById.size();
}

@Override
public PageInfo get(PageId pageId) {
METRICS.incRequest(bookId.getName(), cacheName, GET);
Nikita-Smirnov-Exactpro marked this conversation as resolved.
Show resolved Hide resolved
return pageById.get(pageId);
}

@Override
public PageInfo find(Instant timestamp) {
METRICS.incRequest(bookId.getName(), cacheName, FIND);
Entry<Instant, PageInfo> result = pageByInstant.floorEntry(timestamp);
if (result == null) {
return null;
Expand All @@ -514,12 +504,14 @@ public PageInfo find(Instant timestamp) {

@Override
public PageInfo next(Instant startTimestamp) {
METRICS.incRequest(bookId.getName(), cacheName, NEXT);
Entry<Instant, PageInfo> result = pageByInstant.higherEntry(startTimestamp);
return result != null ? result.getValue() : null;
}

@Override
public PageInfo previous(Instant startTimestamp) {
METRICS.incRequest(bookId.getName(), cacheName, PREVIOUS);
Entry<Instant, PageInfo> result = pageByInstant.lowerEntry(startTimestamp);
return result != null ? result.getValue() : null;
}
Expand All @@ -528,6 +520,7 @@ public PageInfo previous(Instant startTimestamp) {
public Stream<PageInfo> stream(@Nonnull Instant leftBoundTimestamp,
@Nonnull Instant rightBoundTimestamp,
@Nonnull Order order) {
METRICS.incRequest(bookId.getName(), cacheName, ITERATE);
Instant start = pageByInstant.floorKey(leftBoundTimestamp);
NavigableMap<Instant, PageInfo> subMap = pageByInstant.subMap(
start == null ? leftBoundTimestamp : start, true,
Expand Down Expand Up @@ -569,7 +562,7 @@ private static PageInterval create(BookId id, Instant start, BookInfoMetrics.Cac
}
LOGGER.debug("Loaded {} pages for the book {}, {}", pages.size(), id, start);
return new PageInterval(
cacheName,
id, cacheName,
unmodifiableMap(pageById),
unmodifiableNavigableMap(pageByInstant)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.github.benmanes.caffeine.cache.RemovalCause;
import io.prometheus.client.Counter;
import io.prometheus.client.Gauge;
import io.prometheus.client.Summary;

import java.util.Map;
Expand All @@ -28,6 +29,13 @@ public class BookInfoMetrics {
private static final String BOOK_LABEL = "book";
private static final String CACHE_NAME_LABEL = "cache";
private static final String INVALIDATE_CAUSE_LABEL = "cause";
private static final Gauge PAGE_CACHE_SIZE_GAUGE = Gauge.build()
.name("cradle_page_cache_size")
.help("Size of page cache")
.labelNames(BOOK_LABEL, CACHE_NAME_LABEL)
.register();

private static final Map<LoadsKey, Gauge.Child> PAGE_CACHE_SIZE_MAP = new ConcurrentHashMap<>();
private static final Counter PAGE_REQUEST_COUNTER = Counter.build()
.name("cradle_page_cache_page_request_total")
.help("Page requests number from cache")
Expand All @@ -49,6 +57,15 @@ public class BookInfoMetrics {

private static final Map<LoadsKey, Summary.Child> PAGE_LOADS_MAP = new ConcurrentHashMap<>();

public void setPageCacheSize(String book, CacheName cacheName, int value) {
if (cacheName == null) {
return;
}
PAGE_CACHE_SIZE_MAP.computeIfAbsent(
new LoadsKey(book, cacheName), key -> PAGE_CACHE_SIZE_GAUGE.labels(key.toLabels())
).set(value);
}

public void incRequest(String book, CacheName cacheName, RequestMethod method) {
if (cacheName == null) {
return;
Expand Down Expand Up @@ -76,7 +93,9 @@ public enum RequestMethod {
GET,
NEXT,
PREVIOUS,
FIND
FIND,
ITERATE,
REFRESH,
}
public enum CacheName {
HOT,
Expand Down
23 changes: 13 additions & 10 deletions cradle-core/src/main/java/com/exactpro/cradle/CradleStorage.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,14 @@
import java.io.IOException;
import java.time.Instant;
import java.time.ZoneOffset;
import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
Expand All @@ -68,6 +68,7 @@
import java.util.stream.Collectors;

import static com.exactpro.cradle.Order.DIRECT;
import static com.exactpro.cradle.Order.REVERSE;
import static com.exactpro.cradle.resultset.EmptyResultSet.emptyResultSet;

/**
Expand Down Expand Up @@ -1547,20 +1548,22 @@ public final CompletableFuture<Void> updateEventStatusAsync(StoredTestEvent even
}

private Instant checkCollisionAndGetPageEnd(BookInfo book, PageToAdd page, Instant defaultPageEnd) throws CradleStorageException {
Iterator<PageInfo> pageIterator = book.getPages(page.getStart().minus(1, ChronoUnit.NANOS), null, DIRECT);
PageInfo pageInBookBeforeStart = pageIterator.hasNext() ? pageIterator.next() : null;
Iterator<PageInfo> reverseIterator = book.getPages(null, page.getStart(), REVERSE);
PageInfo pageBefore = reverseIterator.hasNext() ? reverseIterator.next() : null;

if (pageInBookBeforeStart != null
&& pageInBookBeforeStart.getEnded() != null
&& pageInBookBeforeStart.getEnded().isAfter(page.getStart())) {
if (pageBefore != null
&& pageBefore.getEnded() != null
&& pageBefore.getEnded().isAfter(page.getStart())) {
throw new CradleStorageException(String.format("Can't add new page in book %s, it collided with current page %s %s-%s",
book.getId().getName(),
pageInBookBeforeStart.getName(),
pageInBookBeforeStart.getStarted(),
pageInBookBeforeStart.getEnded()));
pageBefore.getName(),
pageBefore.getStarted(),
pageBefore.getEnded()));
}

return pageIterator.hasNext() ? pageIterator.next().getStarted() : defaultPageEnd;
Iterator<PageInfo> directIterator = book.getPages(page.getStart(), null, DIRECT);
PageInfo pageAfter = directIterator.hasNext() ? directIterator.next() : null;
return pageAfter == null || Objects.equals(pageAfter, pageBefore) ? defaultPageEnd : pageAfter.getStarted();
}

private List<PageInfo> checkAndConvertPages(List<PageToAdd> pages, BookInfo book) throws CradleStorageException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public class BookInfoTest {
pages.add(createPageInfo(current, null));

// Add random gap
int index = RANDOM.nextInt(1, pages.size() - 1);
int index = RANDOM.nextInt(pages.size() - 2) + 1;
PageInfo pageInfo = pages.remove(index);

PAGES = Collections.unmodifiableList(pages);
Expand Down
Loading