Skip to content
This repository has been archived by the owner on Jul 7, 2022. It is now read-only.

Fixes issue with local short circuit (memcpy) path in the RDMA storage #8

Closed
wants to merge 1 commit into from
Closed

Conversation

patrickstuedi
Copy link
Contributor

tier

  • mmaped buffers were previously indexed with wrong LBA in
    RdmaStorageLocalEndpoint

This fixes the CRAIL-3 JIRA ticket
https://issues.apache.org/jira/browse/CRAIL-3

Signed-off-by: Patrick Stuedi pstuedi@apache.org

tier
- mmaped buffers were previously indexed with wrong LBA in
  RdmaStorageLocalEndpoint

This fixes the CRAIL-3 JIRA ticket
https://issues.apache.org/jira/browse/CRAIL-3

Signed-off-by: Patrick Stuedi <pstuedi@apache.org>
Copy link
Contributor

@PepperJo PepperJo left a comment

Choose a reason for hiding this comment

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

lgtm

bufferMap.put(lba, OffHeapBuffer.wrap(mappedBuffer));
lba += mappedBuffer.capacity();
long lba = Long.parseLong(dataFile.getName());
OffHeapBuffer offHeapBuffer = OffHeapBuffer.wrap(mmap(dataFile));
Copy link
Contributor

Choose a reason for hiding this comment

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

@patrickstuedi can you please indent it properly :)

may be you also don't need these two explicit variables, they are just used once. But I understand for clarity it is fine.

Also - just that I understand, the file names are enumerated with proper name/offset. Because before you had lba+=mappedBuffer.capacity() logic.

Otherwise looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, the indentation was issue with my display. Sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

before the hashtable index was a byte offset, now it's just an int index. We could move back to the byte offset by multiplying the lba with the capacity, and then also multiply the aligneLba with the capacity, but that's unnecessary in my mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the actual problem with the previous code was that the lba was created based on the ordering provided by the Java directory iterator which of course doesn't respect the ordering of the filenames with integer based names..there no guaranteed ordering at all.

Copy link
Member

@asqasq asqasq left a comment

Choose a reason for hiding this comment

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

looks good to me

@asfgit asfgit closed this in baab8d0 Feb 28, 2018
raduioanstoica pushed a commit to raduioanstoica/incubator-crail that referenced this pull request Mar 19, 2018
tier
- mmaped buffers were previously indexed with wrong LBA in
  RdmaStorageLocalEndpoint

This fixes the CRAIL-3 JIRA ticket
https://issues.apache.org/jira/browse/CRAIL-3

Close apache#8

Signed-off-by: Patrick Stuedi <pstuedi@apache.org>
Signed-off-by: Adrian Schuepbach <asq@apache.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants