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

Split BlockStoreEventListener.onCommitBlock() #16777

Merged
merged 64 commits into from
Jan 17, 2023

Conversation

voddle
Copy link
Contributor

@voddle voddle commented Jan 16, 2023

What changes are proposed in this pull request?

listener.onCommitBlock() cannot reflect the state difference between commitBlockToLocal and commitBlockToMaster, so some times when commitBlockToMaster failed the listener.onCommitBlock() will still be called. This PR separated the listener.onCommitBlock() to onCommitBlockToLocal() and onCommitBlockToMaster(), and added corresponding UT to PagedBlockStore and MonoBlockStore.

Why are the changes needed?

Please clarify why the changes are needed. For instance,

  1. listener.onCommitBlock() cannot reflect the state difference between commitBlockToLocal and commitBlockToMaster, so some times when commitBlockToMaster failed the listener.onCommitBlock() will still be called. The separation of the listener.onCommitBlock() can eliminate this inconsistency.

Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
No user-facing changes

previous conversation is here since I select incorrect base repo the first time I create this PR...

Copy link
Contributor

@dbw9580 dbw9580 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, only a few minor issues.

Copy link
Contributor

@dbw9580 dbw9580 left a comment

Choose a reason for hiding this comment

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

LGTM

@jiacheliu3
Copy link
Contributor

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit 6fc0701 into Alluxio:master Jan 17, 2023
jja725 pushed a commit to jja725/alluxio that referenced this pull request Jan 27, 2023
### What changes are proposed in this pull request?

`listener.onCommitBlock()` cannot reflect the state difference between
commitBlockToLocal and commitBlockToMaster, so some times when
commitBlockToMaster failed the `listener.onCommitBlock()` will still be
called. This PR separated the `listener.onCommitBlock()` to
`onCommitBlockToLocal()` and `onCommitBlockToMaster()`, and added
corresponding UT to `PagedBlockStore` and `MonoBlockStore`.

### Why are the changes needed?

Please clarify why the changes are needed. For instance,
1. `listener.onCommitBlock()` cannot reflect the state difference
between commitBlockToLocal and commitBlockToMaster, so some times when
commitBlockToMaster failed the `listener.onCommitBlock()` will still be
called. The separation of the `listener.onCommitBlock()` can eliminate
this inconsistency.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
No user-facing changes

previous conversation is
[here](voddle#1) since I select
incorrect base repo the first time I create this PR...

pr-link: Alluxio#16777
change-id: cid-9c367b0cab5dd5347eda873bc3c8ba189626cc75
@Xenorith Xenorith added the area-worker Alluxio worker label Feb 28, 2023
jiacheliu3 pushed a commit to jiacheliu3/alluxio that referenced this pull request May 15, 2023
`listener.onCommitBlock()` cannot reflect the state difference between
commitBlockToLocal and commitBlockToMaster, so some times when
commitBlockToMaster failed the `listener.onCommitBlock()` will still be
called. This PR separated the `listener.onCommitBlock()` to
`onCommitBlockToLocal()` and `onCommitBlockToMaster()`, and added
corresponding UT to `PagedBlockStore` and `MonoBlockStore`.

Please clarify why the changes are needed. For instance,
1. `listener.onCommitBlock()` cannot reflect the state difference
between commitBlockToLocal and commitBlockToMaster, so some times when
commitBlockToMaster failed the `listener.onCommitBlock()` will still be
called. The separation of the `listener.onCommitBlock()` can eliminate
this inconsistency.

Please list the user-facing changes introduced by your change, including
No user-facing changes

previous conversation is
[here](voddle#1) since I select
incorrect base repo the first time I create this PR...

pr-link: Alluxio#16777
change-id: cid-9c367b0cab5dd5347eda873bc3c8ba189626cc75
jiacheliu3 pushed a commit to jiacheliu3/alluxio that referenced this pull request May 15, 2023
`listener.onCommitBlock()` cannot reflect the state difference between
commitBlockToLocal and commitBlockToMaster, so some times when
commitBlockToMaster failed the `listener.onCommitBlock()` will still be
called. This PR separated the `listener.onCommitBlock()` to
`onCommitBlockToLocal()` and `onCommitBlockToMaster()`, and added
corresponding UT to `PagedBlockStore` and `MonoBlockStore`.

Please clarify why the changes are needed. For instance,
1. `listener.onCommitBlock()` cannot reflect the state difference
between commitBlockToLocal and commitBlockToMaster, so some times when
commitBlockToMaster failed the `listener.onCommitBlock()` will still be
called. The separation of the `listener.onCommitBlock()` can eliminate
this inconsistency.

Please list the user-facing changes introduced by your change, including
No user-facing changes

previous conversation is
[here](voddle#1) since I select
incorrect base repo the first time I create this PR...

pr-link: Alluxio#16777
change-id: cid-9c367b0cab5dd5347eda873bc3c8ba189626cc75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-worker Alluxio worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants