Verify data integrity in exchanges by findepi · Pull Request #3438 · trinodb/trino

findepi · 2020-04-15T09:13:49Z

In our testing, a cloud's network proved to be not reliable. We observed
data corruption when transmitting data over TCP.

Verify data integrity to prevent incorrect query results.

Optionally support retries when data corruption is detected.

presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java

presto-main/src/main/java/io/prestosql/operator/HttpPageBufferClient.java

tooptoop4 · 2020-04-15T10:50:03Z

got a rough idea on performance penalty?

presto-main/src/main/java/io/prestosql/operator/HttpPageBufferClient.java

presto-main/src/main/java/io/prestosql/server/PagesResponseWriter.java

martint · 2020-04-15T16:57:55Z

got a rough idea on performance penalty?

It should be minimal. XXHash64 is a very efficient hash. On my Mac, it can process data at a rate of 14 GB per CPU second.

findepi · 2020-04-15T21:43:24Z

CI green except for test-other-modules (started 5h 43m 20s ago)

AC

findepi · 2020-04-16T12:34:23Z

Rebased.

losipiuk

lgtm

presto-main/src/main/java/io/prestosql/server/PagesResponseWriter.java

presto-main/src/test/java/io/prestosql/operator/TestingExchangeHttpClientHandler.java

presto-main/src/main/java/io/prestosql/execution/buffer/PagesSerdeUtil.java

presto-main/src/main/java/io/prestosql/operator/HttpPageBufferClient.java

presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java

presto-main/src/test/java/io/prestosql/operator/MockExchangeRequestProcessor.java

martint · 2020-04-17T14:35:04Z

presto-main/src/main/java/io/prestosql/operator/HttpPageBufferClient.java

How does this surface to the user? It'd be ideal if it could be mapped to a proper error code.

In practise, this is reachable only when checksum verification is OFF. This is then treated as if IO error occurred and retried.

presto-main/src/main/java/io/prestosql/operator/HttpPageBufferClient.java

findepi · 2020-04-21T21:30:43Z

Updated the retry code.

findepi · 2020-04-22T12:31:23Z

Example failure (as visible in the final query state, eg in UI, or console) when running in default mode (ABORT):

io.prestosql.spi.PrestoException: Checksum verification failure on 172.22.0.2 when reading from http://172.22.0.5:8081/v1/task/20200422_115923_00009_y5gz9.2.0/results/0/1: Data corruption, read checksum: 0x000003f7, calculated checksum: 0x20b0610e25ba9db5
	at io.prestosql.operator.HttpPageBufferClient$1.onFailure(HttpPageBufferClient.java:437)
	at com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1052)
	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:834)
Caused by: io.prestosql.operator.HttpPageBufferClient.ChecksumVerificationException: Data corruption, read checksum: 0x000003f7, calculated checksum: 0x20b0610e25ba9db5
	at io.prestosql.operator.HttpPageBufferClient$PageResponseHandler.verifyChecksum(HttpPageBufferClient.java:672)
	at io.prestosql.operator.HttpPageBufferClient$PageResponseHandler.handle(HttpPageBufferClient.java:650)
	at io.prestosql.operator.HttpPageBufferClient$PageResponseHandler.handle(HttpPageBufferClient.java:582)
	at io.airlift.http.client.jetty.JettyResponseFuture.processResponse(JettyResponseFuture.java:99)
	at io.airlift.http.client.jetty.JettyResponseFuture.completed(JettyResponseFuture.java:76)
	at io.airlift.http.client.jetty.BufferingResponseListener.onComplete(BufferingResponseListener.java:90)
	at org.eclipse.jetty.client.ResponseNotifier.notifyComplete(ResponseNotifier.java:218)
	at org.eclipse.jetty.client.ResponseNotifier.notifyComplete(ResponseNotifier.java:210)
	at org.eclipse.jetty.client.HttpReceiver.terminateResponse(HttpReceiver.java:543)
	at org.eclipse.jetty.client.HttpReceiver.terminateResponse(HttpReceiver.java:523)
	at org.eclipse.jetty.client.HttpReceiver.responseSuccess(HttpReceiver.java:486)
	at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.messageComplete(HttpReceiverOverHTTP.java:326)
	at org.eclipse.jetty.http.HttpParser.handleContentMessage(HttpParser.java:580)
	at org.eclipse.jetty.http.HttpParser.parseContent(HttpParser.java:1697)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1526)
	at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.parse(HttpReceiverOverHTTP.java:200)
	at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.process(HttpReceiverOverHTTP.java:141)
	at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.receive(HttpReceiverOverHTTP.java:75)
	at org.eclipse.jetty.client.http.HttpChannelOverHTTP.receive(HttpChannelOverHTTP.java:133)
	at org.eclipse.jetty.client.http.HttpConnectionOverHTTP.onFillable(HttpConnectionOverHTTP.java:156)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:103)
	at org.eclipse.jetty.io.ChannelEndPoint$2.run(ChannelEndPoint.java:117)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:336)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:313)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:171)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:129)
	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:375)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:806)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:938)
	... 1 more

There is a one problem though. Before the query fails, the exception is logged multiple times.
This is because io.prestosql.operator.ExchangeClient#throwIfFailed rethrows the exception.

I don't see any easy fix for this and it is also pre-existing thing (failure mode is new, but the way
failures are handled isn't). I decided to ignore this for now. If the problem is as rare in practise as
it should be, this won't be a problem.

findepi · 2020-04-22T12:42:41Z

In RETRY mode, the following gets logged (on the affected machine)

2020-04-22T18:26:08.280+0545	WARN	page-buffer-client-callback-7	io.prestosql.operator.HttpPageBufferClient	Checksum verification failure on 172.23.0.5 when reading from http://172.23.0.5:8081/v1/task/20200422_124105_00003_8496b.2.1/results/0/3, may be retried: Data corruption, read checksum: 0x000003f5, calculated checksum: 0x20b0610e25ba9db5

losipiuk · 2020-04-22T14:56:10Z

presto-main/src/main/java/io/prestosql/execution/buffer/PagesSerdeUtil.java

nit: aren't we typically ordering methods that usage is first and declaration follows?

updateChecksum is placed right under writeSerializedPage, because they need to stay in sync.
i added a code comment

In our testing, a cloud's network proved to be not reliable. We observed data corruption when transmitting data over TCP between Presto nodes (internal communication unsecured, no compression). Verify data integrity to prevent incorrect query results. Optionally retry when data corruption is detected.

dain · 2020-04-29T00:46:10Z

I skimmed this. Seem good to me.

findepi requested review from dain, electrum, martint and sopel39 April 15, 2020 09:13

cla-bot bot added the cla-signed label Apr 15, 2020

findepi commented Apr 15, 2020

View reviewed changes

presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java Outdated Show resolved Hide resolved

findepi commented Apr 15, 2020

View reviewed changes

presto-main/src/main/java/io/prestosql/operator/HttpPageBufferClient.java Outdated Show resolved Hide resolved

findepi force-pushed the checksum-v3 branch 2 times, most recently from c7459b7 to c8bb331 Compare April 15, 2020 10:09

losipiuk reviewed Apr 15, 2020

View reviewed changes

presto-main/src/main/java/io/prestosql/operator/HttpPageBufferClient.java Outdated Show resolved Hide resolved

losipiuk reviewed Apr 15, 2020

View reviewed changes

presto-main/src/main/java/io/prestosql/server/PagesResponseWriter.java Outdated Show resolved Hide resolved

findepi force-pushed the checksum-v3 branch from 9896377 to ad590e9 Compare April 15, 2020 15:46

findepi force-pushed the checksum-v3 branch from ad590e9 to e563867 Compare April 16, 2020 12:34

findepi assigned electrum and martint and unassigned electrum and martint Apr 16, 2020

losipiuk approved these changes Apr 17, 2020

View reviewed changes

martint requested changes Apr 17, 2020

View reviewed changes

findepi force-pushed the checksum-v3 branch from e563867 to 95b6a32 Compare April 21, 2020 21:29

findepi force-pushed the checksum-v3 branch 2 times, most recently from 1960d08 to 65d5fae Compare April 22, 2020 09:17

findepi mentioned this pull request Apr 22, 2020

Debug multinode worker #3508

Merged

Verify number of deserialized pages

722056c

findepi force-pushed the checksum-v3 branch from 65d5fae to dac2c84 Compare April 22, 2020 13:54

findepi requested review from losipiuk and martint April 22, 2020 13:54

losipiuk reviewed Apr 22, 2020

View reviewed changes

martint approved these changes Apr 22, 2020

View reviewed changes

findepi force-pushed the checksum-v3 branch 2 times, most recently from a6d4aeb to bcc7267 Compare April 22, 2020 18:08

findepi force-pushed the checksum-v3 branch from bcc7267 to 7a97431 Compare April 23, 2020 08:11

dain removed their request for review April 29, 2020 00:46

findepi merged commit 4d80836 into trinodb:master Apr 30, 2020

findepi deleted the checksum-v3 branch April 30, 2020 23:34

findepi mentioned this pull request Apr 30, 2020

Release notes for 333 #3391

Closed

8 tasks

electrum added this to the 333 milestone May 5, 2020

hackeryang mentioned this pull request Aug 15, 2023

Add S3 connection ttl configuration to reduce http server exception #16007

Merged

Conversation

findepi commented Apr 15, 2020

Uh oh!

Uh oh!

Uh oh!

tooptoop4 commented Apr 15, 2020

Uh oh!

Uh oh!

Uh oh!

martint commented Apr 15, 2020

Uh oh!

findepi commented Apr 15, 2020

Uh oh!

findepi commented Apr 16, 2020

Uh oh!

losipiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martint Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

findepi Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

findepi commented Apr 21, 2020

Uh oh!

findepi commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Apr 22, 2020

Uh oh!

losipiuk Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

findepi Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

dain commented Apr 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

findepi commented Apr 22, 2020 •

edited

Loading