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

Added blockpool struct #2602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added blockpool struct #2602

wants to merge 2 commits into from

Conversation

vadlakondaswetha
Copy link
Collaborator

Description

Added blockpool struct which is responsible for creating and maintaining blocks as per the user configuration.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - Added
  3. Integration tests - NA

@vadlakondaswetha vadlakondaswetha requested a review from a team as a code owner October 17, 2024 08:57
@kislaykishore kislaykishore requested review from a team, kislaykishore and tritone and removed request for a team and kislaykishore October 17, 2024 08:58
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.13%. Comparing base (21633e8) to head (10d018d).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2602      +/-   ##
==========================================
+ Coverage   78.09%   78.13%   +0.03%     
==========================================
  Files         108      109       +1     
  Lines       11860    11894      +34     
==========================================
+ Hits         9262     9293      +31     
- Misses       2088     2091       +3     
  Partials      510      510              
Flag Coverage Δ
unittests 78.13% <100.00%> (+0.03%) ⬆️

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.

// createBlock creates a new block.
func (ib *BlockPool) createBlock() (Block, error) {
prot, flags := syscall.PROT_READ|syscall.PROT_WRITE, syscall.MAP_ANON|syscall.MAP_PRIVATE
addr, err := syscall.Mmap(-1, 0, int(ib.blockSize), prot, flags)
Copy link
Collaborator

@ashmeenkaur ashmeenkaur Oct 18, 2024

Choose a reason for hiding this comment

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

Are we doing this purely for performance reasons?
Just to confirm, we also need to take care of deallocating resources at the end of WriteFile operation completion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, can we add freeBlock() function as well so its abstracted here with totalcount and other free related things.

assert.Equal(t.T(), int64(0), block.Size())
}

func (t *BlockPoolTest) TestCreateBlock() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is same as TestGetWhenTotalBlocksIsLessThanThanMaxBlocks
Shall we remove one?

bp, err := InitBlockPool(1024, 10)

require.Nil(t.T(), err)
assert.NotNil(t.T(), bp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:

Suggested change
assert.NotNil(t.T(), bp)
require.NotNil(t.T(), bp)

// BlockPool handles the creation of blocks as per the user configuration.
type BlockPool struct {
// Channel holding free blocks.
blocksCh chan Block
Copy link
Collaborator

Choose a reason for hiding this comment

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

*nit = Can we rename to blocksChannel or similar like freeBlocksChannel instead to signify its usage and helps in readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants