Fix flaky tests in multiple modules due to non-deterministic specification of HashSet and JSON objects #15418

yashdeep97 · 2023-11-23T04:25:41Z

I found some flaky tests using the NonDex Plugin for maven in various modules.

Description

Fixed the following flaky tests:

`processing` module:

org.apache.druid.segment.nested.NestedDataColumnSupplierV4Test#testBasicFunctionality
org.apache.druid.segment.nested.NestedDataColumnSupplierV4Test#testConcurrency
org.apache.druid.segment.nested.NestedDataColumnSupplierTest#testBasicFunctionality
org.apache.druid.segment.nested.NestedDataColumnSupplierTest#testArrayFunctionality
org.apache.druid.segment.nested.NestedDataColumnSupplierTest#testConcurrency
org.apache.druid.segment.virtual.ExpressionVirtualColumnTest#testRequiredColumns
org.apache.druid.java.util.emitter.core.ParametrizedUriEmitterTest#testEmitterWithParametrizedUriExtractor

`services` module:

org.apache.druid.server.router.TieredBrokerHostSelectorTest#testGetAllBrokers

`indexing-service` module:

org.apache.druid.indexing.overlord.TaskLockboxTest#testGetLockedIntervals

`extensions-core/parquet-extensions` module:

org.apache.druid.data.input.parquet.WikiParquetReaderTest#testUint32Datatype
org.apache.druid.data.input.parquet.WikiParquetReaderTest#testWiki
org.apache.druid.data.input.parquet.NestedColumnParquetReaderTest#testNestedColumnSchemalessNestedTestFile
org.apache.druid.data.input.parquet.CompatParquetReaderTest#testBinaryAsString
org.apache.druid.data.input.parquet.CompatParquetReaderTest#testParquetThriftCompat
org.apache.druid.data.input.parquet.CompatParquetReaderTest#testReadNestedArrayStruct
org.apache.druid.data.input.parquet.CompatParquetReaderTest#testProtoStructWithArray
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest#testFlat1FlattenSelectListItem
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest#testFlat1NoFlattenSpec
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest#testFlat1Autodiscover
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest#testFlat1Flatten
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest#testNested1NoFlattenSpec
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest#testNested1Autodiscover
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest#testNested1Flatten
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest#testNested1FlattenSelectListItem
org.apache.druid.data.input.parquet.TimestampsParquetReaderTest#testDateHandling

`server` module:

org.apache.druid.discovery.BaseNodeRoleWatcherTest#testGeneralUseSimulation
org.apache.druid.indexing.overlord.supervisor.SupervisorStatusTest#testJsonAttr
org.apache.druid.metadata.input.SqlEntityTest#testExecuteQuery

Problem:

The above mentioned tests have been reported as flaky (tests assuming deterministic implementation of a non-deterministic specification ) when ran against the NonDex tool.
The tests contain assertions that compare strings created from JSON objects and lists created from HashSets and HashMaps.

However, HashSet does not guarantee the ordering of elements and thus resulting in these flaky tests that assume deterministic implementation of HashSet. Also, some tests check for a specific ordering of elements in JSON strings. JSON objects are equal even if the ordering of the elements in the JSON strings are not equal. This results in flakiness as the Jackson ObjectMapper does not guarantee consistent ordering of the JSON keys.

Thus, when the NonDex tool shuffles the HashSet elements and the JSON keys, it results in test failures:
Errors can be found in this Github issue: #15471

To reproduce run:

mvn -pl <module_name> edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=<test_name>

Fix:

For the tests failing due to inequality in ordering of objects in lists that were created from HashSets or HashMaps, first convert the arraylists to hashSets and then compare the 2 HashSets using assertEquals().

For all the tests failing due to inequality in ordering of keys in JSON strings, first create the JSON node trees and then compare the two trees using assertEquals().

Made these fixes in collaboration with - @same8891, @Rette66, @prathyushreddylpr, @lxb007981

Key changed/added classes in this PR

org.apache.druid.segment.nested.NestedDataColumnSupplierV4Test
org.apache.druid.segment.nested.NestedDataColumnSupplierTest
org.apache.druid.segment.virtual.ExpressionVirtualColumnTest
org.apache.druid.java.util.emitter.core.ParametrizedUriEmitterTest
org.apache.druid.server.router.TieredBrokerHostSelectorTest
org.apache.druid.indexing.overlord.TaskLockboxTest
org.apache.druid.data.input.parquet.WikiParquetReaderTest
org.apache.druid.data.input.parquet.NestedColumnParquetReaderTest
org.apache.druid.data.input.parquet.CompatParquetReaderTest
org.apache.druid.data.input.parquet.FlattenSpecParquetReaderTest
org.apache.druid.data.input.parquet.TimestampsParquetReaderTest
org.apache.druid.discovery.BaseNodeRoleWatcherTest
org.apache.druid.indexing.overlord.supervisor.SupervisorStatusTest
org.apache.druid.metadata.input.SqlEntityTest

This PR has:

been self-reviewed.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

yashdeep97 · 2023-11-23T04:30:03Z

Hi @cryptoe, thanks for reviewing my previous Pull request (#15318)!
Could you please review this one as well?, it's quite similar to the previous one. (I found more flaky tests in other classes of the processing module)

Please let me know if you need additional details. Thanks in advance!

cryptoe · 2023-11-28T11:13:50Z

@yashdeep97 Could you and the other folks working with the https://github.com/TestingResearchIllinois/NonDex raise a single PR with the changes to the module.

yashdeep97 · 2023-11-29T03:29:13Z

Hi @cryptoe,
Absolutely! I will reach out to others who have similar PRs and try to get them to merge into one.
Also, would it be preferable to have 1 PR for each module (eg. processing, sql etc. ) or a combined PR for tests in all modules?

…st and FlattenSpecParquetReaderTest Co-authored-by: lakkidi2 <lakkidi2@fa23-cs527-035.cs.illinois.edu> Co-authored-by: lxb007981 <lxb007981@hotmail.com> Co-authored-by: Prathyush Reddy Lakkidi <prathyushreddylakkidi@Prathyushs-MacBook-Air.local>

…ns, indexing-service modules

yashdeep97 · 2023-12-01T23:15:52Z

Hi @cryptoe,
I have merged all the pending PRs from students in the course, and updated the PR description.
Please let me know if you need further clarification or if any changes are necessary.

yashdeep97 · 2023-12-01T23:34:14Z

Merged changes from the following PRs:

…uetReaderTest.

cryptoe · 2023-12-11T05:57:29Z

...quet-extensions/src/test/java/org/apache/druid/data/input/parquet/BaseParquetReaderTest.java

@@ -39,6 +39,7 @@
 class BaseParquetReaderTest extends InitializedNullHandlingTest
 {
  ObjectWriter DEFAULT_JSON_WRITER = new ObjectMapper().writerWithDefaultPrettyPrinter();
+  protected final ObjectMapper objectMapper = new ObjectMapper();


Nit: you can use the object mapper created on line 42 for line 41 and remove the inline instantiation of the object mapper.

cryptoe · 2023-12-11T05:59:36Z

...et-extensions/src/test/java/org/apache/druid/data/input/parquet/CompatParquetReaderTest.java

-        expectedJsonBinary,
-        DEFAULT_JSON_WRITER.writeValueAsString(sampledAsBinary.get(0).getRawValues())
-    );
+        objectMapper.readTree(expectedJsonBinary), 


Create a json test utils and then use that to do the assert everywhere.
You can put that class in druid core I think.

Hi @cryptoe, Thanks for your feedback, I'll work on making these changes over this weekend

Hi @cryptoe, I'm unable to figure out where to place this new class that does assert after converting it to JSON tree, should I place it under the druid-testing-tools module in extensions-core? (I did not understand what druid core refers to)

cryptoe · 2023-12-11T06:00:10Z

services/src/test/java/org/apache/druid/server/router/TieredBrokerHostSelectorTest.java

-            "mediumBroker", ImmutableList.of(),
-            "coldBroker", ImmutableList.of("coldHost1:8080", "coldHost2:8080"),
-            "hotBroker", ImmutableList.of("hotHost:8080")
+            "mediumBroker", ImmutableSet.of(),


Why is this change required ?

So, it uses servers which is a ConcurrentHashMap to create the list of Hostnames.

druid/services/src/main/java/org/apache/druid/server/router/TieredBrokerHostSelector.java

Line 63 in e373f62

private final ConcurrentHashMap<String, NodesHolder> servers = new ConcurrentHashMap<>();

And ConcurrentHashMap does not preserve the the ordering of the keys, which can result in the order of servers being different that what is expected in the Assert.
For example: we might get ImmutableList.of("coldHost2:8080", "coldHost1:8080") instead of ImmutableList.of("coldHost1:8080", "coldHost2:8080") which will result in the test failing.
Thus, the code incorrectly assumes deterministic ordering of the keys in ConcurrentHashMap.

cryptoe · 2024-01-03T09:00:07Z

Can you also include #15268 if not included already.

kfaraz · 2024-01-11T06:05:24Z

Have all of these tests been reported as flaky by Druid users/devs? I am not sure if that is really the case. These changes seem to be using sets instead of lists in a lot of places and changing equality assertions of json strings to JsonNode objects. Are these changes really needed?

If there is indeed a flaky test, please share links in the PR description to one or more builds where these tests actually failed.

For cases where there is a flake, we should try to figure out exactly what is causing the flakiness and update tests only when we fully understand the intent of the original test.

cc: @yashdeep97 , @cryptoe

github-actions · 2024-03-12T00:16:11Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2024-04-09T00:16:27Z

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

asdf2014 added the Flaky test label Nov 23, 2023

yashdeep97 mentioned this pull request Nov 27, 2023

Detected new tests in processing module for apache/druid TestingResearchIllinois/idoft#1091

Merged

cryptoe mentioned this pull request Nov 28, 2023

fix testWIKI test #15392

Closed

2 tasks

yashdeep97 force-pushed the fix-processing-flaky-tests branch from ac7d6a4 to c56e856 Compare December 1, 2023 01:10

fix flaky tests in processing module

07821df

Update ParametrizedUriEmitterTest.java

0a88c39

yashdeep97 force-pushed the fix-processing-flaky-tests branch from c56e856 to e763047 Compare December 1, 2023 01:11

yashdeep97 marked this pull request as draft December 1, 2023 01:26

yashdeep97 force-pushed the fix-processing-flaky-tests branch from 38ede59 to 73929cb Compare December 1, 2023 01:30

Fix flaky test in WikiParquetReaderTest.java

9d5f403

yashdeep97 force-pushed the fix-processing-flaky-tests branch from 3f4261e to 9d5f403 Compare December 1, 2023 01:50

prathyushreddylpr and others added 2 commits November 30, 2023 19:54

Fix flaky tests in server, services, extensions-core/parquet-extensio…

794a478

…ns, indexing-service modules

github-actions bot added the Area - Ingestion label Dec 1, 2023

yashdeep97 changed the title ~~Fix flaky tests in processing module~~ Fix flaky tests in multiple modules due to non-deterministic specification of HashSet and JSON objects Dec 1, 2023

remove unnessary changes for wrongly claimed flaky test

8578b68

yashdeep97 marked this pull request as ready for review December 1, 2023 23:14

Fix flaky tests in classesCompatParquetReaderTest and FlattenSpecParq…

120291a

…uetReaderTest.

yashdeep97 mentioned this pull request Dec 4, 2023

Detected new tests in server and extensions-core/parquet-extensions module for apache/druid TestingResearchIllinois/idoft#1166

Merged

cryptoe mentioned this pull request Dec 5, 2023

Fixed flaky tests in CompatParquetReaderTest.java and FlattenSpecParquetReaderTest.java with ObjectMapper #15341

Closed

9 tasks

This was referenced Dec 8, 2023

updated pr-data.csv with new PR TestingResearchIllinois/idoft#1206

Closed

Replaced opened PR's with new PR TestingResearchIllinois/idoft#1207

Merged

CaseyPan mentioned this pull request Dec 8, 2023

added collaborative pr to tests TestingResearchIllinois/idoft#1211

Closed

yashdeep97 mentioned this pull request Dec 10, 2023

Update opened status for druid tests TestingResearchIllinois/idoft#1216

Merged

cryptoe reviewed Dec 11, 2023

View reviewed changes

cryptoe mentioned this pull request Jan 3, 2024

fix flaky-test in ParametrizedUriEmitterTest#testEmitterWithParametrizedUriExtractor #15244

Closed

9 tasks

cryptoe mentioned this pull request Jan 3, 2024

Fixed flaky test testDateHandling in TimestampsParquetReaderTest.java with ObjectMapper #15268

Closed

9 tasks

github-actions bot added the stale label Mar 12, 2024

github-actions bot closed this Apr 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky tests in multiple modules due to non-deterministic specification of HashSet and JSON objects #15418

Fix flaky tests in multiple modules due to non-deterministic specification of HashSet and JSON objects #15418

yashdeep97 commented Nov 23, 2023 •

edited

Loading

yashdeep97 commented Nov 23, 2023 •

edited

Loading

cryptoe commented Nov 28, 2023 •

edited

Loading

yashdeep97 commented Nov 29, 2023

yashdeep97 commented Dec 1, 2023

yashdeep97 commented Dec 1, 2023 •

edited

Loading

cryptoe Dec 11, 2023

cryptoe Dec 11, 2023

yashdeep97 Dec 14, 2023

yashdeep97 Dec 17, 2023

cryptoe Dec 11, 2023

yashdeep97 Dec 17, 2023 •

edited

Loading

cryptoe commented Jan 3, 2024

kfaraz commented Jan 11, 2024 •

edited

Loading

github-actions bot commented Mar 12, 2024

github-actions bot commented Apr 9, 2024

Fix flaky tests in multiple modules due to non-deterministic specification of HashSet and JSON objects #15418

Fix flaky tests in multiple modules due to non-deterministic specification of HashSet and JSON objects #15418

Conversation

yashdeep97 commented Nov 23, 2023 • edited Loading

Description

Fixed the following flaky tests:

processing module:

services module:

indexing-service module:

extensions-core/parquet-extensions module:

server module:

Problem:

Fix:

Key changed/added classes in this PR

yashdeep97 commented Nov 23, 2023 • edited Loading

cryptoe commented Nov 28, 2023 • edited Loading

yashdeep97 commented Nov 29, 2023

yashdeep97 commented Dec 1, 2023

yashdeep97 commented Dec 1, 2023 • edited Loading

cryptoe Dec 11, 2023

Choose a reason for hiding this comment

cryptoe Dec 11, 2023

Choose a reason for hiding this comment

yashdeep97 Dec 14, 2023

Choose a reason for hiding this comment

yashdeep97 Dec 17, 2023

Choose a reason for hiding this comment

cryptoe Dec 11, 2023

Choose a reason for hiding this comment

yashdeep97 Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

cryptoe commented Jan 3, 2024

kfaraz commented Jan 11, 2024 • edited Loading

github-actions bot commented Mar 12, 2024

github-actions bot commented Apr 9, 2024

yashdeep97 commented Nov 23, 2023 •

edited

Loading

`processing` module:

`services` module:

`indexing-service` module:

`extensions-core/parquet-extensions` module:

`server` module:

yashdeep97 commented Nov 23, 2023 •

edited

Loading

cryptoe commented Nov 28, 2023 •

edited

Loading

yashdeep97 commented Dec 1, 2023 •

edited

Loading

yashdeep97 Dec 17, 2023 •

edited

Loading

kfaraz commented Jan 11, 2024 •

edited

Loading