-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-24377 MemStoreFlusher throw NullPointerException #1721
Conversation
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share more context of when you see such NPE? Is it reproducible, should we have test conditions? Also, which version were you testing it, line numbers on the stack trace don't seem to match with master version of MemStoreFlusher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@wchevreuil |
Mind explaining more? |
@binlijin , @wchevreuil - I think I got exactly when we will see this issue. Method reclaimMemStoreMemory() Will be called from RSRpcServices every time during a write op (mutate, multi etc). This will check for the memory global barrier and do actions. You can see in this method the flushType will be set and then we add a WAKEUPFLUSH_INSTANCE entry so that the FlushHandler threads get this entry. This will again do watermark check and call flushOneForGlobalPressure but by then the flushType is been set already. But there is another possible thread flow which can also check this barrier. When the FlushHandler wake up time reached and so it poll and see 'fqe' as null, still it check the barrier. Ya by then already the barrier might have reached. (But the other thread did not call reclaimMemStoreMemory and set the state). This time the call to flushOneForGlobalPressure will result in NPE. |
Wait a minute, FlushType is a field of the MemStoreFlusher? This means we can only have one flush ongoing at the same time? |
I think we dont even need this flushType state variable. The FlushHandler is anyway checking the type by calling the isAboveXXX() method (Even if waked up ). So we can pass in the flushType as local variable. Did not read so carefully. But at high level seems we can avoid the state. |
I think we can just remove the field, and pass the flush type to flushOneForGlobalPressure? The write to this field in reclaimMemStoreMemory is just a dumb one, we will always check it in the flush thread... |
Oh yep, exactly. Agree. |
Yes. Good one. |
Yes good points here. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@Apache9 @anoopsjohn @ramkrish86 ok, remove the flushType field. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: Anoop Sam John <anoop.hbase@gmail.com> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: Anoop Sam John <anoop.hbase@gmail.com> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: Anoop Sam John <anoop.hbase@gmail.com> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: Anoop Sam John <anoop.hbase@gmail.com> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: Anoop Sam John <anoop.hbase@gmail.com> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: Anoop Sam John <anoop.hbase@gmail.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 9b3fe09) Change-Id: I3cba54b8c53775ccbf7f35c9a85c340af9e09663
No description provided.