You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/05/22 05:23:55 UTC

[GitHub] [flink] cyq89051127 opened a new pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

cyq89051127 opened a new pull request #12289:
URL: https://github.com/apache/flink/pull/12289


   
   ## What is the purpose of the change
   Handling the NPE for HBase connector.
   
   ## Brief change log
   
   *adding the NULL check before really serializing the value*
   
   
   ## Verifying this change
   
   *(Please pick either of the following options)*
   
   This change is a trivial rework / code cleanup  and tested in my local computer.
   
   
   ## Does this pull request potentially affect one of the following parts:
   
     - Dependencies (does it add or upgrade a dependency): (*no*)
     - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (*no*)
     - The serializers: (*yes*)
     - The runtime per-record code paths (performance sensitive): (*no*)
     - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (*no*)
     - The S3 file system connector: (*no*)
   
   ## Documentation
   
     - Does this pull request introduce a new feature? (*no*)
     - If yes, how is the feature documented? (not documented)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] cyq89051127 commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
cyq89051127 commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r429731685



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       @wuchong ,hi , i check the code, when deserializeToObject called, there could be a same problem. But when i dig deeper, It seems ok. 
   
   There're two place where `deserializeToObject` is called.
   One(I think it's ok) is like the following in `HBaseReadWriteHelper.parseToRow(Result result, Object rowKey`):
   ```java
       if (value != null) {					
             familyRow.setField(q, HBaseTypeUtils.deserializeToObject(value, typeIdx, charset));
        } else {
   	familyRow.setField(q, null);
       }
   ```
   
   The other one also looks good (in `HBaseReadWriteHelper.parseToRow(Result result)`) : 
   
   ```java 
   public Row parseToRow(Result result) {
   		if (rowKeyIndex == -1) {
   			return parseToRow(result, null);
   		} else {
   			Object rowkey = HBaseTypeUtils.deserializeToObject(result.getRow(), rowKeyType, charset);
   			return parseToRow(result, rowkey);
   		}
   	}
   ```
   
   I can't think of a scenario that result.getRow could return a null value because this code is used to parse the returned value form hbase with a scan command. 
   
   Bug as you see, if there's an emtpy value sent to `deserializeToObject`,  It's not a NPE exception. 
    Do you think we should handle the exception in this PR?  If not, i could create another issue to track the exception in `deserializeToObject`.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] wuchong commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r429288738



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       Could you add a dual logic in the `deserializeToObject`? If the input value is EMPTY_BYTES, we can just return null. I guess we will have similar problem in the deserialization side?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] flinkbot edited a comment on pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12289:
URL: https://github.com/apache/flink/pull/12289#issuecomment-632492310


   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "status" : "DELETED",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/167718532",
       "triggerID" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "triggerType" : "PUSH"
     }, {
       "hash" : "561af1ce20b0faef29dff2f893db99a20cd38957",
       "status" : "FAILURE",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/168043961",
       "triggerID" : "561af1ce20b0faef29dff2f893db99a20cd38957",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * 561af1ce20b0faef29dff2f893db99a20cd38957 Unknown: [FAILURE](https://travis-ci.com/github/flink-ci/flink/builds/168043961) 
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run azure` re-run the last Azure build
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] flinkbot edited a comment on pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12289:
URL: https://github.com/apache/flink/pull/12289#issuecomment-632492310


   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "status" : "PENDING",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/167718532",
       "triggerID" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * cda1c81b986bf87f0fd95abfdc60638813a43c0b Travis: [PENDING](https://travis-ci.com/github/flink-ci/flink/builds/167718532) 
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run travis` re-run the last Travis build
    - `@flinkbot run azure` re-run the last Azure build
   </details>


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] wuchong commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r429753647



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       Yes, I think we should better handle in this PR, because the empty bytes (for types exclude bytes and string) are introduced in this PR. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] flinkbot edited a comment on pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12289:
URL: https://github.com/apache/flink/pull/12289#issuecomment-632492310


   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "status" : "SUCCESS",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/167718532",
       "triggerID" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * cda1c81b986bf87f0fd95abfdc60638813a43c0b Travis: [SUCCESS](https://travis-ci.com/github/flink-ci/flink/builds/167718532) 
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run travis` re-run the last Travis build
    - `@flinkbot run azure` re-run the last Azure build
   </details>


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] flinkbot edited a comment on pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12289:
URL: https://github.com/apache/flink/pull/12289#issuecomment-632492310


   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "status" : "SUCCESS",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/167718532",
       "triggerID" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "triggerType" : "PUSH"
     }, {
       "hash" : "561af1ce20b0faef29dff2f893db99a20cd38957",
       "status" : "PENDING",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/168043961",
       "triggerID" : "561af1ce20b0faef29dff2f893db99a20cd38957",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * cda1c81b986bf87f0fd95abfdc60638813a43c0b Travis: [SUCCESS](https://travis-ci.com/github/flink-ci/flink/builds/167718532) 
   * 561af1ce20b0faef29dff2f893db99a20cd38957 Travis: [PENDING](https://travis-ci.com/github/flink-ci/flink/builds/168043961) 
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run travis` re-run the last Travis build
    - `@flinkbot run azure` re-run the last Azure build
   </details>


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] flinkbot commented on pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12289:
URL: https://github.com/apache/flink/pull/12289#issuecomment-632489396


   Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
   to review your pull request. We will use this comment to track the progress of the review.
   
   
   ## Automated Checks
   Last check on commit cda1c81b986bf87f0fd95abfdc60638813a43c0b (Fri May 22 05:25:45 UTC 2020)
   
   **Warnings:**
    * No documentation files were touched! Remember to keep the Flink docs up to date!
    * **This pull request references an unassigned [Jira ticket](https://issues.apache.org/jira/browse/FLINK-17874).** According to the [code contribution guide](https://flink.apache.org/contributing/contribute-code.html), tickets need to be assigned before starting with the implementation work.
   
   
   <sub>Mention the bot in a comment to re-run the automated checks.</sub>
   ## Review Progress
   
   * ❓ 1. The [description] looks good.
   * ❓ 2. There is [consensus] that the contribution should go into to Flink.
   * ❓ 3. Needs [attention] from.
   * ❓ 4. The change fits into the overall [architecture].
   * ❓ 5. Overall code [quality] is good.
   
   Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
    The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
    - `@flinkbot approve all` to approve all aspects
    - `@flinkbot approve-until architecture` to approve everything until `architecture`
    - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
    - `@flinkbot disapprove architecture` to remove an approval you gave earlier
   </details>


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] cyq89051127 commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
cyq89051127 commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r429731685



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       @wuchong ,hi , i check the code, when deserializeToObject called, there could be a same problem. But when i dig deeper, It seems ok. 
   
   There're two place where `deserializeToObject` is called.
   One(I think it's ok) is like the following in `HBaseReadWriteHelper.parseToRow(Result result, Object rowKey`):
   ```java
       if (value != null) {					
             familyRow.setField(q, HBaseTypeUtils.deserializeToObject(value, typeIdx, charset));
        } else {
   	familyRow.setField(q, null);
       }
   ```
   
   The other one also looks good (in `HBaseReadWriteHelper.parseToRow(Result result)`) : 
   
   ```java 
   public Row parseToRow(Result result) {
   		if (rowKeyIndex == -1) {
   			return parseToRow(result, null);
   		} else {
   			Object rowkey = HBaseTypeUtils.deserializeToObject(result.getRow(), rowKeyType, charset);
   			return parseToRow(result, rowkey);
   		}
   	}
   ```
   
   I can't think of a scenario that result.getRow could return a null value because this code is used to parse the returned value form hbase with a scan command. 
   
   Bug as you see, if there's an emtpy value sent to `deserializeToObject`,  It's not a NPE exception ?  Do you think we should handle the exception in this PR?
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] wuchong commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r431556847



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       We introduced a `null-string-literal` in the new hbase connector (see `HBaseDynamicTableFactory`) to handle this problem. I think we can introduce a same option in the old hbase connector.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] cyq89051127 commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
cyq89051127 commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r429839220



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       @wuchong Hi,  handle the empty bytes in `deserializeToObject` under your suggestion. Please help review. Many thanks.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] cyq89051127 commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
cyq89051127 commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r430279519



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       @wuchong , Hi, when i handle the empty byte by returning a null value, Some tests go wrong, like 'testTableSourceSinkWithDDL','testTableSink' . Because they expect an empty string when the value is empty bytes.  Here we have two options :
   
   1. **Only return a null value when the type is not string (typeIdx ==1), for string type , return an empty string "".**
   ```java
   if (Arrays.equals(value, EMPTY_BYTES)){
   	if (typeIdx == 1){
   		return "";
   	}else{
   		return null;
   	}
   }
   ```
   2. **modifying the test case; making the expect null other than empty string when the value of an column in hbase is empty bytes**
   
   I think we'd better modifying the test case, because when flink writes a null to hbase, we should read it from hbase as null. Even for an existing hbase table, if the value of a column is empty bytes , we take it  as null .  It also works in the same logic. 
   
   How do you think about it?
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] wuchong commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r429740951



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       The rowkey from HBase will never be null or emtpy bytes, but the qualifiers will be an empty bytes because that's what we write in the `serializeFromObject`.

##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       The rowkey from HBase will never be null or emtpy bytes, but the qualifiers might be an empty bytes because that's what we write in the `serializeFromObject`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] cyq89051127 commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
cyq89051127 commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r429731685



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       @wuchong ,hi , i check the code, when deserializeToObject called, there could be a same problem. But when i dig deeper, It seems ok. 
   
   There're two place where `deserializeToObject` is called.
   One(I think it's ok) is like the following in `HBaseReadWriteHelper.parseToRow(Result result, Object rowKey`):
   ```java
       if (value != null) {					
             familyRow.setField(q, HBaseTypeUtils.deserializeToObject(value, typeIdx, charset));
        } else {
   	familyRow.setField(q, null);
       }
   ```
   
   The other one also looks good (in `HBaseReadWriteHelper.parseToRow(Result result)`) : 
   
   ```java 
   public Row parseToRow(Result result) {
   		if (rowKeyIndex == -1) {
   			return parseToRow(result, null);
   		} else {
   			Object rowkey = HBaseTypeUtils.deserializeToObject(result.getRow(), rowKeyType, charset);
   			return parseToRow(result, rowkey);
   		}
   	}
   ```
   
   I can't think of a scenario that result.getRow could return a null value because this code is used to parse the returned value form hbase with a scan command. 
   Do you have any suggestion?
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] flinkbot edited a comment on pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12289:
URL: https://github.com/apache/flink/pull/12289#issuecomment-632492310


   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "status" : "SUCCESS",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/167718532",
       "triggerID" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "triggerType" : "PUSH"
     }, {
       "hash" : "561af1ce20b0faef29dff2f893db99a20cd38957",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "561af1ce20b0faef29dff2f893db99a20cd38957",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * cda1c81b986bf87f0fd95abfdc60638813a43c0b Travis: [SUCCESS](https://travis-ci.com/github/flink-ci/flink/builds/167718532) 
   * 561af1ce20b0faef29dff2f893db99a20cd38957 UNKNOWN
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run travis` re-run the last Travis build
    - `@flinkbot run azure` re-run the last Azure build
   </details>


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] flinkbot commented on pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12289:
URL: https://github.com/apache/flink/pull/12289#issuecomment-632492310


   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * cda1c81b986bf87f0fd95abfdc60638813a43c0b UNKNOWN
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run travis` re-run the last Travis build
    - `@flinkbot run azure` re-run the last Azure build
   </details>


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] cyq89051127 commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
cyq89051127 commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r429744678



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       Yeah, i get it. f there's an emtpy value sent to deserializeToObject, It's not a NPE exception.
   
   Do you think we should handle the exception in this PR? 
    If not, i could create another issue to track the exception in deserializeToObject. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] wuchong commented on a change in pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12289:
URL: https://github.com/apache/flink/pull/12289#discussion_r431556847



##########
File path: flink-connectors/flink-hbase/src/main/java/org/apache/flink/addons/hbase/util/HBaseTypeUtils.java
##########
@@ -81,13 +81,16 @@ public static Object deserializeToObject(byte[] value, int typeIdx, Charset stri
 	 * Serialize the Java Object to byte array with the given type.
 	 */
 	public static byte[] serializeFromObject(Object value, int typeIdx, Charset stringCharset) {
+		if (value == null){
+			return EMPTY_BYTES;

Review comment:
       We introduced a `null-string-literal` option in the new hbase connector (see `HBaseDynamicTableFactory`) to handle this problem. I think we can introduce a same option in the old hbase connector.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [flink] flinkbot edited a comment on pull request #12289: [FLINK-17874][Connectors/HBase]Handling the NPE for hbase-connector

Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12289:
URL: https://github.com/apache/flink/pull/12289#issuecomment-632492310


   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "status" : "DELETED",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/167718532",
       "triggerID" : "cda1c81b986bf87f0fd95abfdc60638813a43c0b",
       "triggerType" : "PUSH"
     }, {
       "hash" : "561af1ce20b0faef29dff2f893db99a20cd38957",
       "status" : "FAILURE",
       "url" : "https://travis-ci.com/github/flink-ci/flink/builds/168043961",
       "triggerID" : "561af1ce20b0faef29dff2f893db99a20cd38957",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * 561af1ce20b0faef29dff2f893db99a20cd38957 Travis: [FAILURE](https://travis-ci.com/github/flink-ci/flink/builds/168043961) 
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run travis` re-run the last Travis build
    - `@flinkbot run azure` re-run the last Azure build
   </details>


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org