Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIVE-28603 Fixing Multiple Flaky tests in module serde which are due to non determinism #5527

Merged

Conversation

nikunjagarwal321
Copy link
Contributor

What changes were proposed in this pull request?

Sorting the fields in function getDeclaredNonStaticFields() in ObjectInspectorUtils.java class in order to make the function deterministic. The below flaky tests will be fixed by this.

Why are the changes needed?

Multiple flaky tests were detected in serde module while trying to run the tests using the nondex tool. NonDex is a tool for detecting and debugging wrong assumptions on under-determined Java APIs.

Steps to reproduce flakiness using nondex -

mvn install -pl serde -DskipTests

mvn -pl serde test -Dtest=org.apache.hadoop.hive.serde2.objectinspector.TestProtocolBuffersObjectInspectors#testProtocolBuffersObjectInspectors

mvn -pl serde edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.apache.hadoop.hive.serde2.objectinspector.TestProtocolBuffersObjectInspectors#testProtocolBuffersObjectInspectors

ERROR logs:

[ERROR] Failures: 
[ERROR]   TestProtocolBuffersObjectInspectors.testProtocolBuffersObjectInspectors:71 expected:<test> but was:<[one, two]>
Screenshot 2024-11-02 at 2 48 35 PM

Reason for Failure

The method getDeclaredNonStaticFields(Class<?> c) in class ObjectInspectorUtils.java uses class.getDeclaredFields() method which is nondeterministic as per java docs.

Field[] f = c.getDeclaredFields();

Screenshot 2024-11-02 at 4 26 58 PM

As per the test cases, it is assumed that the return order of the fields will always be the same, which may not be the case.

All the below test cases are impacted because of the same reason :

  • org.apache.hadoop.hive.serde2.objectinspector.TestProtocolBuffersObjectInspectors.testProtocolBuffersObjectInspectors
  • org.apache.hadoop.hive.serde2.objectinspector.TestThriftObjectInspectors.testThriftObjectInspectors
  • org.apache.hadoop.hive.serde2.objectinspector.TestReflectionObjectInspectors.testReflectionObjectInspectors
  • org.apache.hadoop.hive.serde2.columnar.TestLazyBinaryColumnarSerDe.testLazyBinaryColumnarSerDeWithEmptyBinary
  • org.apache.hadoop.hive.serde2.columnar.TestLazyBinaryColumnarSerDe.testSerDeInnerNulls
  • org.apache.hadoop.hive.serde2.lazybinary.TestLazyBinarySerDe.testLazyBinarySerDe
  • org.apache.hadoop.hive.serde2.TestStatsSerde.testLazyBinarySerDe
  • org.apache.hadoop.hive.serde2.lazy.TestLazySimpleSerDe#testSerDeParameters
  • org.apache.hadoop.hive.serde2.binarysortable.TestBinarySortableSerDe#testBinarySortableSerDe

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

The changes were tested by running the unit tests locally. The code is already covered in the above test cases and is actually fixing the flaky tests.

Please let me know if you have any questions or would like to discuss this further for any clarificaitions.

@difin
Copy link
Contributor

difin commented Nov 9, 2024

LGTM, +1.

@difin
Copy link
Contributor

difin commented Nov 10, 2024

@nikunjagarwal321 please retrigger the build, it needs to pass

Copy link

sonarcloud bot commented Nov 10, 2024

@difin difin merged commit 41925d7 into apache:master Nov 12, 2024
4 checks passed
@nikunjagarwal321
Copy link
Contributor Author

Thanks @difin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants