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

Fix data corrupted issue when loading from UFS #18564

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

yuzhu
Copy link
Contributor

@yuzhu yuzhu commented Apr 1, 2024

This fixes a bug in corrupted data files.
Previously, #17497 attempt to solve this issue but only covers the case when creating a UFS reader Notice we only handle block reader. So this may not fix paged Block Reader in 2.x

Please clarify why the changes are needed. For instance,

  1. If you propose a new API, clarify the use case for a new API.
  2. If you fix a bug, describe the bug.

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

  1. change in user-facing APIs

  2. addition or removal of property keys

  3. webui

       pr-link: Alluxio/alluxio#18525
       change-id: cid-bbba0feb29231e70750e5e79da5f405bb591d47a
    

What changes are proposed in this pull request?

Please outline the changes and how this PR fixes the issue.

Why are the changes needed?

Please clarify why the changes are needed. For instance,

  1. If you propose a new API, clarify the use case for a new API.
  2. If you fix a bug, describe the bug.

Does this PR introduce any user facing changes?

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

  1. change in user-facing APIs
  2. addition or removal of property keys
  3. webui

@yuzhu yuzhu requested a review from jja725 April 1, 2024 23:48
A this fix a bug in corrupted data files.
Previously,  Alluxio#17497 attempt to solve this issue but only covers the case when creating a UFS reader
Notice we only handle block reader. So this may not fix `paged Block Reader` in 2.x

Please clarify why the changes are needed. For instance,
  1. If you propose a new API, clarify the use case for a new API.
  2. If you fix a bug, describe the bug.

Please list the user-facing changes introduced by your change, including
  1. change in user-facing APIs
  2. addition or removal of property keys
  3. webui

			pr-link: Alluxio#18525
			change-id: cid-bbba0feb29231e70750e5e79da5f405bb591d47a
* @return metadata of the block or null if the temp block does not exist
*/
@Nullable
TempBlockMeta getTempBlockMeta(long sessionId, long blockId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually not sure we want to change the interface here, we better just update the place we invoke the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

many places, we do not have a reference to the sessionId, that's why I changed it..

* @param blockId the id of the block
* @return metadata of the block or null if the temp block does not exist
*/
@Nullable
TempBlockMeta getTempBlockMeta(long sessionId, long blockId);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor

@jja725 jja725 left a comment

Choose a reason for hiding this comment

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

Looks like we are not using sessionId anyway in TieredBlockStore so LGTM

@yuzhu
Copy link
Contributor Author

yuzhu commented Apr 2, 2024

alluxio.master.journal.raft.SnapshotReplicationManagerTest not related to the changes

@yuzhu
Copy link
Contributor Author

yuzhu commented Apr 2, 2024

alluxio-bot, force-merge this please

@alluxio-bot
Copy link
Contributor

merge failed:
Merge refused because pull request does not have label start with type-

@yuzhu yuzhu added the type-bug This issue is about a bug label Apr 2, 2024
@yuzhu
Copy link
Contributor Author

yuzhu commented Apr 2, 2024

alluxio-bot, force-merge this please

@alluxio-bot alluxio-bot merged commit ecdd1ea into Alluxio:branch-2.8 Apr 2, 2024
29 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug This issue is about a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants