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

AVRO-4078: [Java] Add the test classes as test case #3216

Draft
wants to merge 2 commits into
base: branch-1.11
Choose a base branch
from

Conversation

martin-g
Copy link
Member

DO NOT MERGE

What is the purpose of the change

The reproducer fails against Avro 1.11.4 but passes against 1.12.0.

The test case passes against 1.11.5-SNAPSHOT and 1.13.0-SNAPSHOT.

Verifying this change

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Extended interop tests to verify consistent valid schema names between SDKs
  • Added test that validates that Java throws an AvroRuntimeException on invalid binary data
  • Manually verified the change by building the website and checking the new redirect

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@martin-g martin-g marked this pull request as draft October 17, 2024 08:32
@github-actions github-actions bot added the Java Pull Requests for Java binding label Oct 17, 2024
@martin-g martin-g changed the base branch from branch-1.11 to main October 17, 2024 08:47
@martin-g martin-g changed the base branch from main to branch-1.12 October 17, 2024 08:48
@martin-g martin-g changed the base branch from branch-1.12 to branch-1.11 October 17, 2024 08:48
public class FullName extends SpecificRecordBase implements SpecificRecord {
private static final long serialVersionUID = 4560514203639509981L;
public static final Schema SCHEMA$ = (new Schema.Parser()).parse(
"{\"type\":\"record\",\"name\":\"FullName\",\"namespace\":\"com.example\",\"fields\":[{\"name\":\"first\",\"type\":{\"type\":\"string\",\"avro.java.string\":\"String\"}},{\"name\":\"last\",\"type\":{\"type\":\"string\",\"avro.java.string\":\"String\"}}]}");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you change the namespace here from com.example to org.apache.avro

@Test
public void testClassLoad() {
System.err.println(FULLNAME_SCHEMA);
System.err.println(SpecificData.get().getClass(FULLNAME_SCHEMA));
Copy link

@pacificmist0900 pacificmist0900 Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add assertNotNull(SpecificData.get().getClass(FULLNAME_SCHEMA)) to make sure that the test only passes when its successfully able to load the class.
Right now the test may/will pass even when it gets a null response from SpecificData.get().getClass(FULLNAME_SCHEMA)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!
This shows the problem in the test case too!
The static initialization of FullName READER$ and WRITER$ fields lead to a hang!

Now the test hangs while trying to initialize the static field READER$
and WRITER$

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g
Copy link
Member Author

diff --git i/lang/java/avro/src/main/java/org/apache/avro/util/ClassUtils.java w/lang/java/avro/src/main/java/org/apache/avro/util/ClassUtils.java
index dad59a551..e042d7b95 100644
--- i/lang/java/avro/src/main/java/org/apache/avro/util/ClassUtils.java
+++ w/lang/java/avro/src/main/java/org/apache/avro/util/ClassUtils.java
@@ -92,7 +92,7 @@ public class ClassUtils {
     Class<?> c = null;
     if (classLoader != null && className != null) {
       try {
-        c = Class.forName(className, true, classLoader);
+        c = Class.forName(className, false, classLoader);
       } catch (ClassNotFoundException e) {
         // Ignore and return null
       }

this fixes the problem and all other tests pass.
I'll leave it to the Java developers to decide whether it is a good solution or not.

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

Successfully merging this pull request may close these issues.

2 participants