You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by jonmeredith <gi...@git.apache.org> on 2018/10/18 17:43:52 UTC
[GitHub] cassandra pull request #288: In BufferPool, make allocating thread receive a...
GitHub user jonmeredith opened a pull request:
https://github.com/apache/cassandra/pull/288
In BufferPool, make allocating thread receive a Chunk.
Prevents a condition where the thread allocating the macro chunk could have all of the chunks taken from the queue before it is able to use one.
This PR is currently pointed at the 3.0 branch, although I noticed that the fix to protect against unexpected mega chunk allocation failures (CASSANDRA-11710 / commit 31cab36b180) is only merged against cassandra-3.2 and cassandra-3.11 when it seems like it would benefit the 3.0 series too.
Please let me know if I should retarget against a later release. The only downside of pointing it at 3.0 is that the merge into 3.11 / trunk requires changing the return introduced in 11710 from `false` to `null`.
For CASSANDRA-14832.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jonmeredith/cassandra CASSANDRA-14832
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cassandra/pull/288.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #288
----
commit e611d400f46127ac73e20cae1e1c4a856d6807a1
Author: Jon Meredith <jm...@...>
Date: 2018-10-18T15:56:46Z
In BufferPool, make allocating thread receive a Chunk.
Prevents a condition where the thread allocating the macro chunk
could have all of the chunks taken from the queue before it
is able to use one.
For CASSANDRA-14832
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #288: In BufferPool, make allocating thread receive a...
Posted by belliottsmith <gi...@git.apache.org>.
Github user belliottsmith commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/288#discussion_r228448169
--- Diff: src/java/org/apache/cassandra/utils/memory/BufferPool.java ---
@@ -237,23 +237,25 @@ void check()
/** Return a chunk, the caller will take owership of the parent chunk. */
public Chunk get()
{
- while (true)
- {
- Chunk chunk = chunks.poll();
- if (chunk != null)
- return chunk;
+ Chunk chunk = chunks.poll();
+ if (chunk != null)
+ return chunk;
- if (!allocateMoreChunks())
- // give it one last attempt, in case someone else allocated before us
- return chunks.poll();
- }
+ chunk = allocateMoreChunks();
+ if (chunk != null)
+ return chunk;
+
+ /* another thread may have just allocated last macro chunk, so
+ ** make one final attempt before returning null
--- End diff --
Super minor nit, with way more verbiage than necessary to follow (on my part), but this is the first time I've seen this style of code comment in the codebase.
The norm is usually to use one of the two basic forms - short info comments just using // for each line, and larger explanatory comments using the standard:
/** (this line typically left blank, for balance with terminating line, but not essential)
*
*
*/
I'm sure we don't stick to this everywhere, and it's not super important which style of code comment you use, but it would be preferable to stick to one of the two most standard formats.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra issue #288: In BufferPool, make allocating thread receive a Chunk.
Posted by belliottsmith <gi...@git.apache.org>.
Github user belliottsmith commented on the issue:
https://github.com/apache/cassandra/pull/288
Yep, LGTM +1
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra issue #288: In BufferPool, make allocating thread receive a Chunk.
Posted by jonmeredith <gi...@git.apache.org>.
Github user jonmeredith commented on the issue:
https://github.com/apache/cassandra/pull/288
@belliottsmith now #279 is merged, if you have time would you mind checking I've read the code right and this correctly makes the change we discussed?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #288: In BufferPool, make allocating thread receive a...
Posted by jonmeredith <gi...@git.apache.org>.
Github user jonmeredith commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/288#discussion_r236362932
--- Diff: src/java/org/apache/cassandra/utils/memory/BufferPool.java ---
@@ -237,23 +237,25 @@ void check()
/** Return a chunk, the caller will take owership of the parent chunk. */
public Chunk get()
{
- while (true)
- {
- Chunk chunk = chunks.poll();
- if (chunk != null)
- return chunk;
+ Chunk chunk = chunks.poll();
+ if (chunk != null)
+ return chunk;
- if (!allocateMoreChunks())
- // give it one last attempt, in case someone else allocated before us
- return chunks.poll();
- }
+ chunk = allocateMoreChunks();
+ if (chunk != null)
+ return chunk;
+
+ /* another thread may have just allocated last macro chunk, so
+ ** make one final attempt before returning null
--- End diff --
thx for the pointers, updated to a single line.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org