[ https://issues.apache.org/jira/browse/HBASE-26688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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)