[jira] [Comment Edited] (HBASE-26688) Threads shared EMPTY_RESULT may lead to unexpected client job down.
[
https://issues.apache.org/jira/browse/HBASE-26688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17484009#comment-17484009
]
Andrew Kyle Purtell edited comment on HBASE-26688 at 1/28/22, 11:09 PM:
bq. So I prefer we include the fix here in branch-2.5+, on branch-2.4 we just
keep the old behavior.
I basically agree with this approach, but can we do this?
- In branch-2.4, no changes
- In branch-2.5 and branch-2, keep relevant exception in the method signature
(if it is a checked exception) but change the behavior. Release 2.5.0 will
include a release note that describes the change.
- In master, change the behavior and it's also fine to modify the method
signature.
And if you opt for this:
bq. Perhaps we can have a follow-on that makes the API more consistent, and
roll that change out in accordance with our obligations.
Then that would be fine, but
- In branch-2 / branch-2.5, additive changes only. Keep old method signatures.
- In master branch and 3.x versions, breaking changes are ok.
was (Author: apurtell):
bq. So I prefer we include the fix here in branch-2.5+, on branch-2.4 we just
keep the old behavior.
I basically agree with this approach, but can we do this?
- In branch-2.4, no changes
- In branch-2.5 and branch-2, keep relevant exception in the method signature
(if it is a checked exception) but change the behavior. Release 2.5.0 will
include a release note that describes the change.
- In master, change the behavior and it's also fine to modify the method
signature.
And if you opt for this:
bq. Perhaps we can have a follow-on that makes the API more consistent, and
roll that change out in accordance with our obligations.
Then that would be fine, but
- In branch-2, additive changes only. Keep old method signatures.
- In master branch and 3.x versions, breaking changes are ok.
> Threads shared EMPTY_RESULT may lead to unexpected client job down.
> ---
>
> Key: HBASE-26688
> URL: https://issues.apache.org/jira/browse/HBASE-26688
> Project: HBase
> Issue Type: Bug
> Components: Client
>Reporter: Yutong Xiao
>Assignee: Yutong Xiao
>Priority: Major
> Fix For: 2.5.0, 3.0.0-alpha-3, 2.4.10
>
>
> Currently, we use a pre-created EMPTY_RESULT in the ProtoBuf.util to reduce
> the object creation. But these objects could be shared by multi client
> threads. The Result#cellScannerIndex related methods could throw confusing
> exception and make the client job down. Could refine the logic of these
> methods.
> The precreated objects in ProtoBufUtil.java:
> {code:java}
> private final static Cell[] EMPTY_CELL_ARRAY = new Cell[]{};
> private final static Result EMPTY_RESULT = Result.create(EMPTY_CELL_ARRAY);
> final static Result EMPTY_RESULT_EXISTS_TRUE = Result.create(null, true);
> final static Result EMPTY_RESULT_EXISTS_FALSE = Result.create(null, false);
> private final static Result EMPTY_RESULT_STALE =
> Result.create(EMPTY_CELL_ARRAY, null, true);
> {code}
> Result#advance
> {code:java}
> public boolean advance() {
> if (cells == null) return false;
> cellScannerIndex++;
> if (cellScannerIndex < this.cells.length) {
> return true;
> } else if (cellScannerIndex == this.cells.length) {
> return false;
> }
> // The index of EMPTY_RESULT could be incremented by multi threads and
> throw this exception.
> throw new NoSuchElementException("Cannot advance beyond the last cell");
> }
> {code}
> Result#current
> {code:java}
> public Cell current() {
> if (cells == null
> || cellScannerIndex == INITIAL_CELLSCANNER_INDEX
> || cellScannerIndex >= cells.length)
> return null;
>// Although it is almost impossible,
>// We can arrive here when the client threads share the common reused
> EMPTY_RESULT.
> return this.cells[cellScannerIndex];
> }
> {code}
> In this case, the client can easily got confusing exceptions even if they use
> different connections, tables in different threads.
> We should change the if condition cells == null to isEmpty() to avoid the
> client crashed from these exception.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Comment Edited] (HBASE-26688) Threads shared EMPTY_RESULT may lead to unexpected client job down.
[
https://issues.apache.org/jira/browse/HBASE-26688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17484009#comment-17484009
]
Andrew Kyle Purtell edited comment on HBASE-26688 at 1/28/22, 11:08 PM:
bq. So I prefer we include the fix here in branch-2.5+, on branch-2.4 we just
keep the old behavior.
I basically agree with this approach, but can we do this?
- In branch-2.4, no changes
- In branch-2.5 and branch-2, keep relevant exception in the method signature
(if it is a checked exception) but change the behavior. Release 2.5.0 will
include a release note that describes the change.
- In master, change the behavior and it's also fine to modify the method
signature.
And if you opt for this:
bq. Perhaps we can have a follow-on that makes the API more consistent, and
roll that change out in accordance with our obligations.
Then that would be fine, but
- In branch-2, additive changes only. Keep old method signatures.
- In master branch and 3.x versions, breaking changes are ok.
was (Author: apurtell):
bq. So I prefer we include the fix here in branch-2.5+, on branch-2.4 we just
keep the old behavior.
I basically agree with this approach, but can we do this?
- In branch-2.4, no changes
- In branch-2.5 and branch-2, keep relevant exception in the method signature
(if it is a checked exception) but change the behavior. Release 2.5.0 will
include a release note that describes the change.
- In master, change the behavior and it's also fine to modify the method
signature.
> Threads shared EMPTY_RESULT may lead to unexpected client job down.
> ---
>
> Key: HBASE-26688
> URL: https://issues.apache.org/jira/browse/HBASE-26688
> Project: HBase
> Issue Type: Bug
> Components: Client
>Reporter: Yutong Xiao
>Assignee: Yutong Xiao
>Priority: Major
> Fix For: 2.5.0, 3.0.0-alpha-3, 2.4.10
>
>
> Currently, we use a pre-created EMPTY_RESULT in the ProtoBuf.util to reduce
> the object creation. But these objects could be shared by multi client
> threads. The Result#cellScannerIndex related methods could throw confusing
> exception and make the client job down. Could refine the logic of these
> methods.
> The precreated objects in ProtoBufUtil.java:
> {code:java}
> private final static Cell[] EMPTY_CELL_ARRAY = new Cell[]{};
> private final static Result EMPTY_RESULT = Result.create(EMPTY_CELL_ARRAY);
> final static Result EMPTY_RESULT_EXISTS_TRUE = Result.create(null, true);
> final static Result EMPTY_RESULT_EXISTS_FALSE = Result.create(null, false);
> private final static Result EMPTY_RESULT_STALE =
> Result.create(EMPTY_CELL_ARRAY, null, true);
> {code}
> Result#advance
> {code:java}
> public boolean advance() {
> if (cells == null) return false;
> cellScannerIndex++;
> if (cellScannerIndex < this.cells.length) {
> return true;
> } else if (cellScannerIndex == this.cells.length) {
> return false;
> }
> // The index of EMPTY_RESULT could be incremented by multi threads and
> throw this exception.
> throw new NoSuchElementException("Cannot advance beyond the last cell");
> }
> {code}
> Result#current
> {code:java}
> public Cell current() {
> if (cells == null
> || cellScannerIndex == INITIAL_CELLSCANNER_INDEX
> || cellScannerIndex >= cells.length)
> return null;
>// Although it is almost impossible,
>// We can arrive here when the client threads share the common reused
> EMPTY_RESULT.
> return this.cells[cellScannerIndex];
> }
> {code}
> In this case, the client can easily got confusing exceptions even if they use
> different connections, tables in different threads.
> We should change the if condition cells == null to isEmpty() to avoid the
> client crashed from these exception.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Comment Edited] (HBASE-26688) Threads shared EMPTY_RESULT may lead to unexpected client job down.
[
https://issues.apache.org/jira/browse/HBASE-26688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17483509#comment-17483509
]
Yutong Xiao edited comment on HBASE-26688 at 1/28/22, 1:33 AM:
---
For non-empty Result objects, the logic is the same after the change.
This only changes the behaviour of Result with empty cell list, which used to
have a thread safety bug.
A more common usage is to check the return value of Result#advance once first,
if it is false, the client should search the next result.
This usage is impacted by this bug. Even if the client wants to catch the
exception, they may get the exception at the first time of advance() but not
the second time, which is not the original logic.
By the way, in the original code, if the cell list is null, the method also
always returns false and never raises the exception.
Result#isEmpty with null cell list and Result#isEmpty with empty list all
return true. But in the original code, "empty" Results have different
behaviours. Should we also care about this?
This is actually a bug.
If we want to keep return NoSuchElementException for Result with empty list. My
opinion is we can change the Result class thread safe. (e.g. make
Result#cellScannerIndex threadlocal?)
was (Author: xytss123):
For non-empty Result objects, the logic is the same after the change.
This only changes the behaviour of Result with empty cell list, which used to
have a thread safety bug.
A more common usage is to check the return value of Result#advance once first,
if it is false, the client should search the next result.
This usage is impacted by this bug. Even if the client wants to catch the
exception, they may get the exception at the first time of advance() but not
the second time, which is not the original logic.
By the way, in the original code, if the cell list is null, the method also
always returns false and never raises the exception.
Result#isEmpty with null cell list and Result#isEmpty with empty list all
return true. But in the original code, "empty" Results have different
behaviours.
This is actually a bug.
If we want to keep return NoSuchElementException for Result with empty list. My
opinion is we can change the Result class thread safe. (e.g. make
Result#cellScannerIndex threadlocal?)
> Threads shared EMPTY_RESULT may lead to unexpected client job down.
> ---
>
> Key: HBASE-26688
> URL: https://issues.apache.org/jira/browse/HBASE-26688
> Project: HBase
> Issue Type: Bug
> Components: Client
>Reporter: Yutong Xiao
>Assignee: Yutong Xiao
>Priority: Major
> Fix For: 2.5.0, 3.0.0-alpha-3, 2.4.10
>
>
> Currently, we use a pre-created EMPTY_RESULT in the ProtoBuf.util to reduce
> the object creation. But these objects could be shared by multi client
> threads. The Result#cellScannerIndex related methods could throw confusing
> exception and make the client job down. Could refine the logic of these
> methods.
> The precreated objects in ProtoBufUtil.java:
> {code:java}
> private final static Cell[] EMPTY_CELL_ARRAY = new Cell[]{};
> private final static Result EMPTY_RESULT = Result.create(EMPTY_CELL_ARRAY);
> final static Result EMPTY_RESULT_EXISTS_TRUE = Result.create(null, true);
> final static Result EMPTY_RESULT_EXISTS_FALSE = Result.create(null, false);
> private final static Result EMPTY_RESULT_STALE =
> Result.create(EMPTY_CELL_ARRAY, null, true);
> {code}
> Result#advance
> {code:java}
> public boolean advance() {
> if (cells == null) return false;
> cellScannerIndex++;
> if (cellScannerIndex < this.cells.length) {
> return true;
> } else if (cellScannerIndex == this.cells.length) {
> return false;
> }
> // The index of EMPTY_RESULT could be incremented by multi threads and
> throw this exception.
> throw new NoSuchElementException("Cannot advance beyond the last cell");
> }
> {code}
> Result#current
> {code:java}
> public Cell current() {
> if (cells == null
> || cellScannerIndex == INITIAL_CELLSCANNER_INDEX
> || cellScannerIndex >= cells.length)
> return null;
>// Although it is almost impossible,
>// We can arrive here when the client threads share the common reused
> EMPTY_RESULT.
> return this.cells[cellScannerIndex];
> }
> {code}
> In this case, the client can easily got confusing exceptions even if they use
> different connections, tables in different threads.
> We should change the if condition cells == null to isEmpty() to avoid the
> client crashed from these exception.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Comment Edited] (HBASE-26688) Threads shared EMPTY_RESULT may lead to unexpected client job down.
[
https://issues.apache.org/jira/browse/HBASE-26688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17483509#comment-17483509
]
Yutong Xiao edited comment on HBASE-26688 at 1/28/22, 1:33 AM:
---
For non-empty Result objects, the logic is the same after the change.
This only changes the behaviour of Result with empty cell list, which used to
have a thread safety bug.
A more common usage is to check the return value of Result#advance once first,
if it is false, the client should search the next result.
This usage is impacted by this bug. Even if the client wants to catch the
exception, they may get the exception at the first time of advance() but not
the second time, which is not the original logic.
By the way, in the original code, if the cell list is null, the method also
always returns false and never raises the exception.
Result#isEmpty with null cell list and Result#isEmpty with empty list all
return true. But in the original code, "empty" Results have different
behaviours. Should we also care about this?
If we want to keep return NoSuchElementException for Result with empty list. My
opinion is we can change the Result class thread safe. (e.g. make
Result#cellScannerIndex threadlocal?)
was (Author: xytss123):
For non-empty Result objects, the logic is the same after the change.
This only changes the behaviour of Result with empty cell list, which used to
have a thread safety bug.
A more common usage is to check the return value of Result#advance once first,
if it is false, the client should search the next result.
This usage is impacted by this bug. Even if the client wants to catch the
exception, they may get the exception at the first time of advance() but not
the second time, which is not the original logic.
By the way, in the original code, if the cell list is null, the method also
always returns false and never raises the exception.
Result#isEmpty with null cell list and Result#isEmpty with empty list all
return true. But in the original code, "empty" Results have different
behaviours. Should we also care about this?
This is actually a bug.
If we want to keep return NoSuchElementException for Result with empty list. My
opinion is we can change the Result class thread safe. (e.g. make
Result#cellScannerIndex threadlocal?)
> Threads shared EMPTY_RESULT may lead to unexpected client job down.
> ---
>
> Key: HBASE-26688
> URL: https://issues.apache.org/jira/browse/HBASE-26688
> Project: HBase
> Issue Type: Bug
> Components: Client
>Reporter: Yutong Xiao
>Assignee: Yutong Xiao
>Priority: Major
> Fix For: 2.5.0, 3.0.0-alpha-3, 2.4.10
>
>
> Currently, we use a pre-created EMPTY_RESULT in the ProtoBuf.util to reduce
> the object creation. But these objects could be shared by multi client
> threads. The Result#cellScannerIndex related methods could throw confusing
> exception and make the client job down. Could refine the logic of these
> methods.
> The precreated objects in ProtoBufUtil.java:
> {code:java}
> private final static Cell[] EMPTY_CELL_ARRAY = new Cell[]{};
> private final static Result EMPTY_RESULT = Result.create(EMPTY_CELL_ARRAY);
> final static Result EMPTY_RESULT_EXISTS_TRUE = Result.create(null, true);
> final static Result EMPTY_RESULT_EXISTS_FALSE = Result.create(null, false);
> private final static Result EMPTY_RESULT_STALE =
> Result.create(EMPTY_CELL_ARRAY, null, true);
> {code}
> Result#advance
> {code:java}
> public boolean advance() {
> if (cells == null) return false;
> cellScannerIndex++;
> if (cellScannerIndex < this.cells.length) {
> return true;
> } else if (cellScannerIndex == this.cells.length) {
> return false;
> }
> // The index of EMPTY_RESULT could be incremented by multi threads and
> throw this exception.
> throw new NoSuchElementException("Cannot advance beyond the last cell");
> }
> {code}
> Result#current
> {code:java}
> public Cell current() {
> if (cells == null
> || cellScannerIndex == INITIAL_CELLSCANNER_INDEX
> || cellScannerIndex >= cells.length)
> return null;
>// Although it is almost impossible,
>// We can arrive here when the client threads share the common reused
> EMPTY_RESULT.
> return this.cells[cellScannerIndex];
> }
> {code}
> In this case, the client can easily got confusing exceptions even if they use
> different connections, tables in different threads.
> We should change the if condition cells == null to isEmpty() to avoid the
> client crashed from these exception.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
