Skip to content

Replace null with empty String in master proxy result set.#8557

Merged
morningman merged 1 commit intoapache:masterfrom
Jibing-Li:jibing-dev
Mar 22, 2022
Merged

Replace null with empty String in master proxy result set.#8557
morningman merged 1 commit intoapache:masterfrom
Jibing-Li:jibing-dev

Conversation

@Jibing-Li
Copy link
Copy Markdown
Contributor

@Jibing-Li Jibing-Li commented Mar 21, 2022

Replace null with empty String in master proxy result set. This is to fix NPE when the master proxy result set contains a null item. The NPE could cause the thrift server crash and fail to send the result back to the client which is a follower or an observer fe.

Problem Summary:

Suppose we have 3 follower FEs, say A, B and C. A is currently the master. If B becomes the master after A crashes, we will get the following error when we run "show frontends" command in the mysql client with connection to C:
ERROR 1105 (HY000): TTransportException, msg: Socket is closed by peer.
which is caused by the thrift server side NPE (ResultSet couldn't contain null):

2022-03-21 15:55:13 ERROR TThreadPoolServer:321 - Error occurred during processing of message.
java.lang.NullPointerException
	at org.apache.thrift.protocol.TBinaryProtocol.writeString(TBinaryProtocol.java:225)
	at org.apache.doris.thrift.TShowResultSet$TShowResultSetStandardScheme.write(TShowResultSet.java:488)
	at org.apache.doris.thrift.TShowResultSet$TShowResultSetStandardScheme.write(TShowResultSet.java:409)
	at org.apache.doris.thrift.TShowResultSet.write(TShowResultSet.java:346)
	at org.apache.doris.thrift.TMasterOpResult$TMasterOpResultStandardScheme.write(TMasterOpResult.java:637)
	at org.apache.doris.thrift.TMasterOpResult$TMasterOpResultStandardScheme.write(TMasterOpResult.java:562)
	at org.apache.doris.thrift.TMasterOpResult.write(TMasterOpResult.java:480)
	at org.apache.doris.thrift.FrontendService$forward_result$forward_resultStandardScheme.write(FrontendService.java:14456)
	at org.apache.doris.thrift.FrontendService$forward_result$forward_resultStandardScheme.write(FrontendService.java:14418)
	at org.apache.doris.thrift.FrontendService$forward_result.write(FrontendService.java:14369)
	at org.apache.thrift.ProcessFunction.process(ProcessFunction.java:58)
	at org.apache.thrift.TBaseProcessor.process(TBaseProcessor.java:38)
	at org.apache.thrift.server.TThreadPoolServer$WorkerProcess.run(TThreadPoolServer.java:313)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)

Checklist(Required)

  1. Does it affect the original behavior: No
  2. Has unit tests been added: No Need
  3. Has document been added or modified: No Need
  4. Does it need to update dependencies: No
  5. Are there any changes that cannot be rolled back: No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@yiguolei
Copy link
Copy Markdown
Contributor

LGTM

ArrayList<String> list = Lists.newArrayList();
for (int j = 0; j < resultRows.get(i).size(); j ++) {
list.add(resultRows.get(i).get(j));
list.add(resultRows.get(i).get(j) == null ? "" : resultRows.get(i).get(j));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use FeConstants.null_string instead.

… fix NPE when the master proxy result set contains a null item. The NPE could cause the thrift server crash and failed to send the result back to the client which is a follower or observer fe.
Copy link
Copy Markdown
Contributor

@hf200012 hf200012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Mar 21, 2022
@github-actions
Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@morningman morningman added the dev/1.0.1-deprecated should be merged into dev-1.0.1 branch label Mar 21, 2022
@morningman morningman merged commit 9a0a1c6 into apache:master Mar 22, 2022
@morningman morningman added dev/merged-1.0.1-deprecated PR has been merged into dev-1.0.1 and removed dev/1.0.1-deprecated should be merged into dev-1.0.1 branch labels Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. dev/merged-1.0.1-deprecated PR has been merged into dev-1.0.1 reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants