You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "yugvajani (via GitHub)" <gi...@apache.org> on 2024/10/10 22:46:16 UTC
[PR] Fixed the flaky test testSerdeWithInputFormat by sorting the result (druid)
yugvajani opened a new pull request, #17325:
URL: https://github.com/apache/druid/pull/17325
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
Fixed a flaky test in `org.apache.druid.indexing.kafka.supervisor.KafkaSupervisorSpecTest.testSerdeWithInputFormat`
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Steps to reproduce
To reproduce the problem, first build the module `kafka-indexing-service`:
> mvn install -pl extensions-core/kafka-indexing-service -am -DskipTests
Then, run the regular test:
> mvn -pl extensions-core/kafka-indexing-service test -Dtest=org.apache.druid.indexing.kafka.supervisor.KafkaSupervisorSpecTest#testSerdeWithInputFormat
To identify the flaky test, execute the following [nondex](https://github.com/TestingResearchIllinois/NonDex) command:
> mvn -pl extensions-core/kafka-indexing-service edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.apache.druid.indexing.kafka.supervisor.KafkaSupervisorSpecTest#testSerdeWithInputFormat
### Description
The testSerdeWithInputFormat was producing non-deterministic output because the JSON object used in the test did not include a parser that enforced a specific order, unlike other tests. Due to the inherent non-deterministic nature of JSON serialization, this resulted in inconsistent outputs. The fix involves sorting the serialized JSON to ensure a consistent order.
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
* `sortJsonNode` - This function is the sort function to sort the objectNode and arrayNode inside the Json object.
* `sortArrayNode` - This function is called to sort the arrayNode.
* `sortJsonString` - Deserializes JsonString to JsonNode , sorts it and serializes it back to JsonString.
The sortJsonString function can now be directly used to sort the serialized Json ,which will ensure a deterministic order for all tests where the serialized Json gives a non deterministic order.
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Functions added in this PR
* `sortJsonNode`
* `sortArrayNode`
* `sortJsonString`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the non deterministic test testSerdeWithInputFormat (druid)
Posted by "yugvajani (via GitHub)" <gi...@apache.org>.
yugvajani commented on code in PR #17325:
URL: https://github.com/apache/druid/pull/17325#discussion_r1829824424
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpecTest.java:
##########
@@ -310,71 +317,115 @@ public void testSerdeWithInputFormat() throws IOException
KafkaSupervisorSpec spec2 = mapper.readValue(serialized, KafkaSupervisorSpec.class);
String stable = mapper.writeValueAsString(spec2);
+ String sortedStableJson = sortJsonString(stable);
Review Comment:
@kgyrtkirk Do you have some progress on inspecting this? I have a similar change for `org.apache.druid.indexing.kafka.supervisor.KafkaSupervisorSpecTest.testSerdeWithSpecAndInputFormat` and don't know whether to add to this PR or open another PR. 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the non deterministic test testSerdeWithInputFormat (druid)
Posted by "yugvajani (via GitHub)" <gi...@apache.org>.
yugvajani commented on code in PR #17325:
URL: https://github.com/apache/druid/pull/17325#discussion_r1819284799
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpecTest.java:
##########
@@ -310,71 +317,115 @@ public void testSerdeWithInputFormat() throws IOException
KafkaSupervisorSpec spec2 = mapper.readValue(serialized, KafkaSupervisorSpec.class);
String stable = mapper.writeValueAsString(spec2);
+ String sortedStableJson = sortJsonString(stable);
Review Comment:
`could you point to the field which's comparision is the root cause of these failures? are there any options there to alter the production code to be more stable?` - The output of `dimensionSpec `was non-deterministic because the parser wasn’t added to `dimensionSpec`, which led to ordering issues. I’ve implemented a fix by adding a parser, similar to how other tests are set up, which resolves the non-deterministic ordering problem. I’ve updated the PR with this fix—please review it and let me know if it looks good. Now, the array's ordering remains unchanged, but the dimension order is now deterministic.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the non deterministic test testSerdeWithInputFormat (druid)
Posted by "yugvajani (via GitHub)" <gi...@apache.org>.
yugvajani commented on PR #17325:
URL: https://github.com/apache/druid/pull/17325#issuecomment-2544314507
Initially, I had resolved the issue by sorting the contents of the JSON. However, since the ordering of elements is important, I later updated the solution to align with how other tests handled ordering. The parser we were using provided a deterministic order, which ensured the test passed.
Now that the parser is deprecated, one possible solution could be to avoid relying on string equality for comparisons and instead perform a direct JSON comparison. What is your opinion on this one?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the flaky test testSerdeWithInputFormat by sorting the result (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17325:
URL: https://github.com/apache/druid/pull/17325#discussion_r1819081198
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpecTest.java:
##########
@@ -310,71 +317,115 @@ public void testSerdeWithInputFormat() throws IOException
KafkaSupervisorSpec spec2 = mapper.readValue(serialized, KafkaSupervisorSpec.class);
String stable = mapper.writeValueAsString(spec2);
+ String sortedStableJson = sortJsonString(stable);
Review Comment:
Could you point to a test failure when this happened without instrumentation?
> The sort function I created could be used as a general-purpose tool to ensure deterministic ordering across tests
I don't think that would be a good path forward - it reorders arrays without thinking about that may semantically be different...its ok to reorder if the array is say argument to an `IN` ; but if they are arguments to a function they may mean a different thing in a different order.
I think to fix these things for sure - the source should be addressed and not add extra lenient comparision stuff to the tests.
I'm not sure I was able to find the answers for my previous questions above:
* could you point to the field which's comparision is the root cause of these failures? are there any options there to alter the production code to be more stable?
* have you considered/evaluated making the `KafkaSupervisorSpec`/etc comparable - so that instead of resorting to string comparision application level comparision can be made?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the non deterministic test testSerdeWithInputFormat (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on PR #17325:
URL: https://github.com/apache/druid/pull/17325#issuecomment-2548399041
I think this is not a test-only issue; its more like the system fails to return stable results
I think it should be tracked down how the inconsistency could happen with the non-deprecated codepath.
Seems like the comparision of `dimensionExclusions` fails? where is that field declared? what typed object it gets?
you will be most likely be able stabilize the test by making changes like `HashSet` to `LinkedHashSet` or something similar - you may also need to update the test; but after that it will not happen anymore.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the non deterministic test testSerdeWithInputFormat (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17325:
URL: https://github.com/apache/druid/pull/17325#issuecomment-2727082516
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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the flaky test testSerdeWithInputFormat by sorting the result (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17325:
URL: https://github.com/apache/druid/pull/17325#discussion_r1796514318
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpecTest.java:
##########
@@ -310,71 +317,115 @@ public void testSerdeWithInputFormat() throws IOException
KafkaSupervisorSpec spec2 = mapper.readValue(serialized, KafkaSupervisorSpec.class);
String stable = mapper.writeValueAsString(spec2);
+ String sortedStableJson = sortJsonString(stable);
Review Comment:
note: this seems like an unconditional resort at the Json level for only this test; but I wonder if it only affects this
from the repro steps: `nondex` have detected that the output here is unstable.
I don't know the Kafka part; but there might be lists/arrays for which order does matter - sorting all parts of a json unconditionally may possible hide issues.
I wonder if it was investigated what causes these mismatches. I suspect that some underlying class may use some unstable constructs - in most cases these used to boil down to a `HashSet -> LinkedHashSet` change...
Another possible way to fix this in a wider scope is to make the `KafkaSupervisorSpec` comparable and do the validation at that level; so that the real classes can decide if 2 maps/sets are equal or not ; and lists / arrays will still be validated correctly
what do you think about these approaches?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the non deterministic test testSerdeWithInputFormat (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on PR #17325:
URL: https://github.com/apache/druid/pull/17325#issuecomment-2505724322
I'm a little puzzled seeing that the `parser` seem to have been [deprecated](https://github.com/apache/druid/blob/c1ef38b0526554bceedd78001aed8e9fb86ef126/server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java#L96-L102) ; and the PR which did introduced `timespampSpec` was the one deprecating that in #8823.
If the fix would be to move from a deprecated one to a non-deprecated one I wouldn't had any questions/etc...but its the other way around
do you know how the inconsistency could happen with the non-deprecated codepath? the comparision of which field leads to the issue?
I still feel like this is just masking the issue and not stabilizes the non-deprecated system to be "stable" - so that it produces the same results with different java versions/etc.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the flaky test testSerdeWithInputFormat by sorting the result (druid)
Posted by "yugvajani (via GitHub)" <gi...@apache.org>.
yugvajani commented on code in PR #17325:
URL: https://github.com/apache/druid/pull/17325#discussion_r1803946175
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpecTest.java:
##########
@@ -310,71 +317,115 @@ public void testSerdeWithInputFormat() throws IOException
KafkaSupervisorSpec spec2 = mapper.readValue(serialized, KafkaSupervisorSpec.class);
String stable = mapper.writeValueAsString(spec2);
+ String sortedStableJson = sortJsonString(stable);
Review Comment:
If an underlying class were using HashSet, other tests in the same file would likely have failed as well, since they all use the same underlying class. The key difference with testSerdeWithInputFormat is that it wasn’t using a parser, which is probably what makes the list order deterministic in the other tests.
I tried adding the parser to this test, and while it did fix the order, I ran into an issue where Assert.assertEquals(4, spec.getDataSchema().getAggregators().length); returned a length of 0. I'm not entirely sure why, as I’m not very familiar with the Kafka-specific logic.
The sort function I created could be used as a general-purpose tool to ensure deterministic ordering across tests, if needed. In fact, most of the tests only rely on assert.contains, except for one AssertEquals. Since the sort function only arranges the list elements and doesn’t alter their content, it shouldn’t cause problems, especially since the parser itself was handling ordering in other cases.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the non deterministic test testSerdeWithInputFormat (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #17325: Fixed the non deterministic test testSerdeWithInputFormat
URL: https://github.com/apache/druid/pull/17325
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fixed the non deterministic test testSerdeWithInputFormat (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17325:
URL: https://github.com/apache/druid/pull/17325#issuecomment-2661161296
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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org