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

Introduced data freshness with the MongoDB implementation at connection-level and at query-level #209

Merged
merged 11 commits into from
Sep 19, 2024

Conversation

suresh-prakash
Copy link
Contributor

@suresh-prakash suresh-prakash commented Sep 13, 2024

This PRs gives the flexibility to redirect reads optionally to secondary MongoDB nodes.
This configuration can be tweaked at the connection-level while instantiating a connection-config object. It can also be configured at a query level. By default, all the reads will go to the primary node for backward compatibility.

Preference order:

  1. Query level configuration
  2. Connection level configuration if query level configuration is default (unspecified)
  3. Document store defaults to reads from primary (if both query-level and connection-level read preferences are unspecified/default).

In this PR, the Postgres implementation simply ignores the configuration.

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.69%. Comparing base (7e161f5) to head (e85b786).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...tore/model/config/mongo/MongoConnectionConfig.java 0.00% 0 Missing and 2 partials ⚠️
...mongo/collection/MongoReadPreferenceConverter.java 81.81% 2 Missing ⚠️
...core/documentstore/model/options/QueryOptions.java 75.00% 1 Missing ⚠️
...race/core/documentstore/mongo/MongoCollection.java 0.00% 1 Missing ⚠️
...ongo/collection/MongoCollectionOptionsApplier.java 91.66% 0 Missing and 1 partial ⚠️
...ore/documentstore/postgres/PostgresCollection.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #209      +/-   ##
============================================
- Coverage     79.70%   79.69%   -0.01%     
- Complexity     1022     1036      +14     
============================================
  Files           194      198       +4     
  Lines          4879     4921      +42     
  Branches        408      411       +3     
============================================
+ Hits           3889     3922      +33     
- Misses          706      713       +7     
- Partials        284      286       +2     
Flag Coverage Δ
integration 79.69% <83.33%> (-0.01%) ⬇️
unit 57.50% <81.25%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Sep 13, 2024

Test Results

 39 files  +1   39 suites  +1   34s ⏱️ +2s
242 tests +7  242 ✅ +7  0 💤 ±0  0 ❌ ±0 
495 runs  +7  495 ✅ +7  0 💤 ±0  0 ❌ ±0 

Results for commit e85b786. ± Comparison against base commit 7e161f5.

♻️ This comment has been updated with latest results.

@@ -6,7 +6,7 @@ plugins {
id("org.hypertrace.ci-utils-plugin") version "0.3.0"
id("org.hypertrace.publish-plugin") version "1.0.2" apply false
id("org.hypertrace.code-style-plugin") version "1.1.0" apply false
id("org.owasp.dependencycheck") version "8.2.1"
id("org.owasp.dependencycheck") version "8.4.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for not moving to latest version - 10.0.4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that version. It's failing (mostly due to Java version incompatibility).

@@ -71,7 +71,7 @@ public class DocStoreTest {
public static void init() {
datastoreMap = Maps.newHashMap();
mongo =
new GenericContainer<>(DockerImageName.parse("mongo:4.4.0"))
new GenericContainer<>(DockerImageName.parse("mongo:7.0.14"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the MongoDB version 7.0.14? Is this the same version that has been deployed?

* @param queryOptions The query options
* @return {@link CloseableIterator} of matching documents
*/
CloseableIterator<Document> query(
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: the existing query api deprecated?

Copy link
Contributor Author

@suresh-prakash suresh-prakash Sep 18, 2024

Choose a reason for hiding this comment

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

Don't think that's required. If clients want to query with the default configurations, the older method can be used.

@SuppressWarnings("UnnecessarySemicolon")
public enum DataFreshness {
SYSTEM_DEFAULT,
REALTIME_FRESHNESS,
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: What does the freshness mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Freshness means the liveliness of the data.
REALTIME_FRESHNESS means we want the most up-to-date data.
NEAR_REALTIME_FRESHNESS means a little bit of stale data is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here NEAR_REALTIME_FRESHNESS means fetching data from secondary, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Basically "SECONDARY" is preferred. In case "SECONDARY" is not available, it'll fetch from "PRIMARY".

final CacheKey cacheKey =
CacheKey.builder().readPreference(readPreference(connectionConfig, queryOptions)).build();
return collectionCache.computeIfAbsent(
cacheKey, key -> collection.withReadPreference(key.readPreference()));
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the CacheKey associated with the collection? Since ReadPreference can be either primary or secondary, does this mean that this.collectionCache contains a maximum of 2 values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since ReadPreference can be either primary or secondary, does this mean that this.collectionCache contains a maximum of 2 values?

Yes.

@SuppressWarnings("UnnecessarySemicolon")
public enum DataFreshness {
SYSTEM_DEFAULT,
REALTIME_FRESHNESS,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here NEAR_REALTIME_FRESHNESS means fetching data from secondary, right?

@suresh-prakash suresh-prakash merged commit 72cc0d2 into main Sep 19, 2024
6 of 7 checks passed
@suresh-prakash suresh-prakash deleted the introduce_data_freshness_parameter branch September 19, 2024 06:36
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