Skip to content

feat: java frontend passes epoch to cn #406

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 21, 2022
Merged

feat: java frontend passes epoch to cn #406

merged 4 commits into from
Feb 21, 2022

Conversation

@zwang28
Copy link
Contributor

@zwang28 zwang28 commented Feb 17, 2022 •
edited
Loading

What's changed and what's your intention?

  • Batch query requires an epoch which must has been committed in hummock, so that all compute nodes involved read a consistent hummock snapshot. RowSeqScan in current codebase is an example.
  • This PR modifies the QueryHandler in java frontend to pass a newest cached committed epoch to compute nodes. Queries in one frontend guarantee monotonic reads.
  • The new HummockSnapshotManager in java frontend maintains a cache of committed epochs, and it fetches latest one from the meta periodically (100ms). It also ensures the epoch being used in a query remains valid (data within this epoch won't be deleted in a hummock compaction).

TODO:

  • Set frontend's node id in HummockSnapshotManager, which is used as pin_snapshot/unpin_snapshot RPC parameter. If the frontend is removed from cluster, its pinned snapshots will be removed (safe guard).
  • To perform monotonic reads across multiple frontends, a query must force an update in epoch cache first.

Checklist

Refer to a related PR or issue link (optional)

close #384

@zwang28 zwang28 changed the title feat: frontend uses epoch (WIP)feat: java frontend uses epoch Feb 17, 2022
@github-actions github-actions bot added the type/feature Type: New feature. label Feb 17, 2022
@zwang28 zwang28 changed the title (WIP)feat: java frontend uses epoch (WIP)feat: java frontend passes epoch to cn Feb 17, 2022
@github-actions github-actions bot removed the type/feature Type: New feature. label Feb 17, 2022
@zwang28 zwang28 force-pushed the wangzheng/feat_java_frontend_uses_epoch branch 2 times, most recently from dfc2e27 to 7725946 Compare February 18, 2022 03:02
@zwang28 zwang28 marked this pull request as ready for review February 18, 2022 03:29
@zwang28 zwang28 marked this pull request as draft February 18, 2022 03:36
@zwang28 zwang28 force-pushed the wangzheng/feat_java_frontend_uses_epoch branch 2 times, most recently from 0a1bcb3 to 5b11365 Compare February 18, 2022 04:55
@codecov
Copy link

codecov bot commented Feb 18, 2022 •
edited
Loading

Codecov Report

Merging #406 (ad9e3e0) into main (c495af8) will decrease coverage by 0.10%.
The diff coverage is 11.60%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #406      +/-   ##
============================================
- Coverage     73.12%   73.01%   -0.11%     
- Complexity     2709     2711       +2     
============================================
  Files           864      869       +5     
  Lines         48411    48492      +81     
  Branches       1551     1553       +2     
============================================
+ Hits          35399    35406       +7     
- Misses        12203    12277      +74     
  Partials        809      809
Flag Coverage Δ
java 63.52% <11.60%> (-0.36%) ⬇️
rust 77.08% <ø> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...c/main/kotlin/com/risingwave/rpc/GrpcMetaClient.kt 0.00% <0.00%> (ø)
.../com/risingwave/pgserver/FrontendServerModule.java 0.00% <0.00%> (ø)
...risingwave/execution/context/ExecutionContext.java 89.47% <0.00%> (-2.42%) ⬇️
...com/risingwave/execution/handler/QueryHandler.java 0.00% <0.00%> (ø)
...gwave/execution/handler/cache/EpochComparator.java 0.00% <0.00%> (ø)
...xecution/handler/cache/HummockSnapshotManager.java 0.00% <0.00%> (ø)
...tion/handler/cache/HummockSnapshotManagerImpl.java 0.00% <0.00%> (ø)
...ngwave/execution/handler/cache/ScopedSnapshot.java 0.00% <0.00%> (ø)
...tion/handler/cache/MockHummockSnapshotManager.java 33.33% <33.33%> (ø)
.../com/risingwave/execution/context/FrontendEnv.java 88.88% <66.66%> (-4.87%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c495af8...ad9e3e0. Read the comment docs.

@zwang28 zwang28 force-pushed the wangzheng/feat_java_frontend_uses_epoch branch from 5b11365 to e720bd7 Compare February 18, 2022 05:36
@zwang28 zwang28 force-pushed the wangzheng/feat_java_frontend_uses_epoch branch from e720bd7 to ee41d22 Compare February 18, 2022 06:11
@zwang28 zwang28 marked this pull request as ready for review February 18, 2022 06:28
@zwang28 zwang28 changed the title (WIP)feat: java frontend passes epoch to cn feat: java frontend passes epoch to cn Feb 18, 2022
@zwang28 zwang28 requested a review from skyzh February 18, 2022 06:45
@github-actions github-actions bot added the type/feature Type: New feature. label Feb 18, 2022
skyzh
skyzh approved these changes Feb 21, 2022
View reviewed changes
Copy link
Contributor

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

The overall logic LGTM. But I'm not an expert on Java, and we need another reviewer for this PR.

java/pgserver/src/main/java/com/risingwave/pgserver/FrontendServerModule.java
LOGGER.info("Creating hummock snapshot manager.");
String address = config.get(FrontendServerConfigurations.META_SERVICE_ADDRESS);
DefaultWorkerNode node = DefaultWorkerNode.from(address);
MetaClient client =
Copy link
Contributor

@skyzh skyzh Feb 21, 2022

Choose a reason for hiding this comment

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

Why not share the same meta client instance with createStreamManager?

Copy link
Contributor Author

@zwang28 zwang28 Feb 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I prefer MetaClientManager to be passed to callers who need MetaClient. The MetaClientManager should handle underlying channel reconnection stuffs, and the callers just calls getOrCreate to obtain a client using up-to-date channel each time.
But as channel reconnection is not in current codebase and we are moving towards rust frontend, a shared meta client instance is fine.

@zwang28 zwang28 requested review from fuyufjh and removed request for BugenZhao February 21, 2022 02:50
liurenjie1024
liurenjie1024 approved these changes Feb 21, 2022
View reviewed changes
java/meta/src/main/kotlin/com/risingwave/rpc/GrpcMetaClient.kt Outdated

import com.risingwave.common.exception.PgErrorCode
import com.risingwave.common.exception.PgException
import com.risingwave.proto.hummock.*
Copy link
Contributor

@liurenjie1024 liurenjie1024 Feb 21, 2022

Choose a reason for hiding this comment

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

It's not good practice to import * in java.

@zwang28 zwang28 merged commit 2a314a3 into main Feb 21, 2022
@zwang28 zwang28 deleted the wangzheng/feat_java_frontend_uses_epoch branch February 21, 2022 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BugenZhao BugenZhao BugenZhao approved these changes

@neverchanje neverchanje Awaiting requested review from neverchanje

@fuyufjh fuyufjh Awaiting requested review from fuyufjh

+2 more reviewers

@liurenjie1024 liurenjie1024 liurenjie1024 approved these changes

@skyzh skyzh skyzh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type/feature Type: New feature.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

frontend pass latest committed epoch via create_task RPC

5 participants

@zwang28 @liurenjie1024 @skyzh @BugenZhao