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

Conversation

Nikita-Smirnov-Exactpro
Copy link
Member

No description provided.

@Nikita-Smirnov-Exactpro Nikita-Smirnov-Exactpro marked this pull request as ready for review February 22, 2024 09:57
}

@Test(dataProvider = "cacheSize")
public void lazyPageAddTest(int cacheSize) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a small note: what did we break if this test fails? I tried to figure that out for some time and the only answer I had was 'something with pages'. In my personal opinion, unit tests should be as concise as possible and answer the question: what did we break? And this test does not answer this question.
This is not a request to change it immediately, just a piece of thought about what unit tests should look like.

Copy link
Member Author

Choose a reason for hiding this comment

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

This test was made as hot check. I will split it to several part at the end of task

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

# Conflicts:
#	README.md
#	cradle-cassandra/build.gradle
#	cradle-cassandra/src/main/java/com/exactpro/cradle/cassandra/dao/messages/AbstractMessageIteratorProvider.java
#	cradle-cassandra/src/main/java/com/exactpro/cradle/cassandra/dao/messages/CassandraStoredMessageFilter.java
#	cradle-cassandra/src/main/java/com/exactpro/cradle/cassandra/dao/messages/MessageBatchesIteratorProvider.java
#	cradle-cassandra/src/main/java/com/exactpro/cradle/cassandra/dao/messages/MessagesIteratorProvider.java
#	gradle.properties
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@Nikita-Smirnov-Exactpro Nikita-Smirnov-Exactpro merged commit 40c0478 into dev-version-5 Mar 6, 2024
8 of 9 checks passed
@Nikita-Smirnov-Exactpro Nikita-Smirnov-Exactpro deleted the TH2-5165 branch March 6, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants