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

Replacing Jackson Factory with Gson Factory and further code updation #32158

Merged
merged 5 commits into from
Aug 19, 2024

Conversation

cutiepie-10
Copy link
Contributor

@cutiepie-10 cutiepie-10 commented Aug 12, 2024

Hi Reviewer,
I have tried to resolve #32130 and tried to work on the changes as you have discussed on my previous pull request.
I have found some more places in the code where JacksonFactory was used and successfully replaced them.

Sorry for adding so many changes with one commit.

Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me, just a couple minor comments and then we should be able to merge!

gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
@@ -136,7 +136,7 @@ public static void tearDownTempBucket() throws IOException {
request.setReadTimeout(60000); // 1 minute read timeout
};
Storage storage =
new Storage.Builder(new NetHttpTransport(), new JacksonFactory(), requestInitializer)
new Storage.Builder(new NetHttpTransport(), new GsonFactory(), requestInitializer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is now failing code formatting checks, could you please run: ./gradlew :sdks:java:io:google-cloud-platform:spotlessApply from the root of the project to fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran this on the cmd line but the build failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2024-08-15 21-24-41

May be this is happening because of I am missing some project requirements

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried going through https://github.com/apache/beam/blob/master/CONTRIBUTING.md#setup-your-environment-and-learn-about-language-specific-setup? In particular, the ./local-env-setup.sh piece might help

If you don't want to deal with the dependencies for this, you can also just use the suggestion from the build itself - https://github.com/apache/beam/actions/runs/10406736496/job/28820457976?pr=32158

In this case, the relevant piece is:

Execution failed for task ':sdks:java:io:google-cloud-platform:spotlessJavaCheck'.
> The following files had format violations:
      sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HttpHealthcareApiClient.java
          @@ -734,8 +734,7 @@
           ????????????????CloudHealthcareScopes.CLOUD_PLATFORM,?StorageScopes.CLOUD_PLATFORM_READ_ONLY));
           
           ????client?=
          -????????new?CloudHealthcare.Builder(
          -????????????????new?NetHttpTransport(),?new?GsonFactory(),?requestInitializer)
          +????????new?CloudHealthcare.Builder(new?NetHttpTransport(),?new?GsonFactory(),?requestInitializer)
           ????????????.setApplicationName("apache-beam-hl7v2-io")
           ????????????.build();
           ????httpClient?=
      sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/healthcare/FhirIOTestUtil.java
          @@ -136,8 +136,7 @@
           ??????????request.setReadTimeout(60000);?//?1?minute?read?timeout
           ????????};
           ????Storage?storage?=
          -????????new?Storage.Builder(new?NetHttpTransport(),?new?GsonFactory(),?requestInitializer)
          -????????????.build();
          +????????new?Storage.Builder(new?NetHttpTransport(),?new?GsonFactory(),?requestInitializer).build();
           ????List<StorageObject>?blobs?=?storage.objects().list(DEFAULT_TEMP_BUCKET).execute().getItems();
           ????if?(blobs?!=?null)?{
           ??????for?(StorageObject?blob?:?blobs)?{
  Run './gradlew :sdks:java:io:google-cloud-platform:spotlessApply' to fix these violations.

So to fix, you'd update the lines in FhirIOTestUtil.java and HttpHealthcareApiClient.java to follow the specified format

Copy link
Contributor Author

@cutiepie-10 cutiepie-10 Aug 15, 2024

Choose a reason for hiding this comment

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

Hi @damccorm,
I perfoormed the setup manually and checked it again but the only error I am getting is building the project.
The build fails while building compileGroovy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, could you please just manually edit the lines then? If you share the error you're getting I may be able to help as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error I am getting is mentioned in the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2024-08-17 10-44-05

This is the error I am getting while running ./local-env-setup.sh and in the comments above I have shown the error I am getting while building the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a bunch of permission errors - could you try running sudo ./local-env-setup.sh and running it as root? Or if you don't want to give elevated permissions to the whole script, you could modify it to just run certain commands as root.

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for sticking with it!

@damccorm damccorm merged commit 8c0fbf7 into apache:master Aug 19, 2024
30 checks passed
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.

[Task]: Replace JacksonFactory references with GsonFactory
3 participants