Skip to content

KAFKA-7707: remove the code never execute#6002

Closed
huangyiminghappy wants to merge 5 commits intoapache:trunkfrom
huangyiminghappy:clean_code
Closed

KAFKA-7707: remove the code never execute#6002
huangyiminghappy wants to merge 5 commits intoapache:trunkfrom
huangyiminghappy:clean_code

Conversation

@huangyiminghappy
Copy link
Copy Markdown
Contributor

@huangyiminghappy huangyiminghappy commented Dec 5, 2018

in the BufferPool,the waiters is locked by ReentrantLock,and the waiters add Condition all within the lock,and the waiters remove also within the lock.in the waiters there is only one Condition instance.
image
and in the finally we have remove the waiters's condition,so in the finally, we use the

finally {
            // signal any additional waiters if there is more memory left
            // over for them
            try {
                if (!(this.nonPooledAvailableMemory == 0 && this.free.isEmpty()) && !this.waiters.isEmpty())
                    this.waiters.peekFirst().signal();
            } finally {
                // Another finally... otherwise find bugs complains
                lock.unlock();
            }
        }

can modify like

finally {
            lock.unlock();
        }

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@huangyiminghappy huangyiminghappy changed the title KAFKA-7707: clean the code never execute KAFKA-7707: remove the code never execute Dec 6, 2018
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Dec 8, 2018

cc @smccauliff

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

@huangyiminghappy I'm a little confused why you think it is not possible to have multiple waiters. When we await the condition, the lock is released. Am I missing something?

@huangyiminghappy
Copy link
Copy Markdown
Contributor Author

@hachikuji thank you review, may be i missing something,i read the BufferPool,found during the lock,only one thread can run,and one condition can add to the waiters,and during the lock ,the condition is remove by waiters, at this time i think this.waiters.peekFirst() is not required.

@huxihx
Copy link
Copy Markdown
Contributor

huxihx commented Dec 20, 2018

@huangyiminghappy The lock will be released when moreMemory.await is called and other thread(s) might add themselves as new waiters. Does it make sense?

@huangyiminghappy
Copy link
Copy Markdown
Contributor Author

@huangyiminghappy The lock will be released when moreMemory.await is called and other thread(s) might add themselves as new waiters. Does it make sense?

thank you,i have not pay attention the moreMemory.await,in deallocate method ,it will signal that notice it may have memory to allocate

@hachikuji
Copy link
Copy Markdown
Contributor

@huangyiminghappy Thanks for the response. It sounds like you are convinced that the current implementation works? Shall we close this or do you think there is still an issue?

@huangyiminghappy
Copy link
Copy Markdown
Contributor Author

entation works? Shall we close this or d

thank you ,i close this issue

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants