Skip to content

Add task retries smoke product test #22798

Merged
losipiuk merged 2 commits intotrinodb:masterfrom
losipiuk:lukaszos/add-task-retries-smoke-product-test-cfbce9
Jul 26, 2024
Merged

Add task retries smoke product test #22798
losipiuk merged 2 commits intotrinodb:masterfrom
losipiuk:lukaszos/add-task-retries-smoke-product-test-cfbce9

Conversation

@losipiuk
Copy link
Copy Markdown
Member

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

losipiuk added 2 commits July 24, 2024 18:46
Add smoke product test run with task retries, using filesystem based
exchange to gaurd from bugs which are not visible in
DistributedQueryRunner (e.g. some classpath related problems).
@cla-bot cla-bot bot added the cla-signed label Jul 24, 2024
@losipiuk losipiuk requested a review from wendigo July 24, 2024 16:46
@martint
Copy link
Copy Markdown
Member

martint commented Jul 24, 2024

Why does this need to be a product test instead of a regular test using the distributed query runner and multiple in-process nodes?

@losipiuk
Copy link
Copy Markdown
Member Author

Why does this need to be a product test instead of a regular test using the distributed query runner and multiple in-process nodes?

Regular test did not catch jackson serialization problems introduced with #22722 (then reverted).
I do not fully understand the root cause - but classloader setup is different in DQR based tests than what we have in actual deployment - so I guess this is the problem.

DQR queries run fine but with product tests we get:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No serializer found for class io.trino.plugin.base.metrics.TDigest and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS) (through reference chain: io.tr
ino.execution.TaskInfo["stats"]->io.trino.operator.TaskStats["pipelines"]->com.google.common.collect.RegularImmutableList[0]->io.trino.operator.PipelineStats["operatorSummaries"]->com.google.common.collect.RegularImmutableList[0]->io.trino.operator.OperatorStats["pipelineMetrics"]->jav
a.util.ImmutableCollections$MapN["FileSystemExchangeSource.s3GetObjectRequestsFailed"]->io.trino.plugin.base.metrics.TDigestHistogram["digest"])
        at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:77)
        at com.fasterxml.jackson.databind.SerializerProvider.reportBadDefinition(SerializerProvider.java:1330)
        at com.fasterxml.jackson.databind.DatabindContext.reportBadDefinition(DatabindContext.java:414)
        at com.fasterxml.jackson.databind.ser.impl.UnknownSerializer.failForEmpty(UnknownSerializer.java:53)
        at com.fasterxml.jackson.databind.ser.impl.UnknownSerializer.serialize(UnknownSerializer.java:30)
        at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:732)
        at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:770)
        at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeWithType(BeanSerializerBase.java:653)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeTypedFields(MapSerializer.java:1027)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:779)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
        at com.fasterxml.jackson.databind.ser.std.JsonValueSerializer.serialize(JsonValueSerializer.java:289)
        at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:732)
        at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:770)

on worker with #22722 applied.

@losipiuk
Copy link
Copy Markdown
Member Author

Why does this need to be a product test instead of a regular test using the distributed query runner and multiple in-process nodes?

Regular test did not catch jackson serialization problems introduced with #22722 (then reverted). I do not fully understand the root cause - but classloader setup is different in DQR based tests than what we have in actual deployment - so I guess this is the problem.
....

#22797 fixes the problem - but it would be great to have regression test coverage.

@losipiuk losipiuk requested a review from martint July 24, 2024 17:11
@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Jul 25, 2024

I don't think that adding a product test is worth it tbh. I'd prefer to create some test that basically creates a plugin class loader and tests whether you can serialize/deserialize class when you are in the server class loader.

@losipiuk
Copy link
Copy Markdown
Member Author

I don't think that adding a product test is worth it tbh. I'd prefer to create some test that basically creates a plugin class loader and tests whether you can serialize/deserialize class when you are in the server class loader.

How would that be different from test which uses DistributedQueryRunner - where serialization works fine for some reason? This PT is extra 2 mins of CI runtime (in a job which is short anyway so does not extend overall CI for PR), so I do not think it is a big deal.

FileAttribute<Set<PosixFilePermission>> posixFilePermissions = PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-r--r--"));
Path minioBucketDirectory;
try {
minioBucketDirectory = Files.createTempDirectory("test-bucket-contents", posixFilePermissions);
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.

Do we need to expose minio data to the host?

@losipiuk losipiuk merged commit b902cfd into trinodb:master Jul 26, 2024
@github-actions github-actions bot added this to the 454 milestone Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants