Skip to content

Conversation

XN137
Copy link
Contributor

@XN137 XN137 commented Jul 23, 2025

all static methods that took a PolarisCallContext as first parameter can be moved to the new class.

if we did not need to log json-related exceptions to PolarisDiagnostics (which we dont do in other places) then we could keep everything as static util methods.

@@ -187,8 +108,4 @@ public int getAttemptCount() {
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you look above this line, json errors around task state dont get reported to PolarisDiagnostics but are simply logged

import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisTaskConstants;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/** A mapper to serialize/deserialize polaris objects. */
public class PolarisObjectMapperUtil {
public final class PolarisObjectMapperUtil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we go forward with this PR we might want to rename this class as it now only deals with task (execution) state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way works for me :)

@@ -41,16 +40,16 @@ public static TaskEntity of(PolarisBaseEntity polarisEntity) {

public <T> T readData(Class<T> klass) {
PolarisCallContext polarisCallContext = CallContext.getCurrentContext().getPolarisCallContext();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if PolarisObjectMapperUtil methods did not require the PolarisDiagnostics for reporting json-related errors, then this usage of CallContext.getCurrentContext() could go away (also below in this file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

found this gem a well:

if (configStr != null) {
return ConnectionConfigInfoDpo.deserialize(new PolarisDefaultDiagServiceImpl(), configStr);
}

Copy link
Contributor

@dimas-b dimas-b 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 like a nice refactoring... Did you mean to remove the "draft" state and open this PR for reviews?

@XN137
Copy link
Contributor Author

XN137 commented Jul 24, 2025

Did you mean to remove the "draft" state and open this PR for reviews?

not yet, i will probably make an alternative PR where i remove PolarisDiagnostics from all json-parsing related helpers and show the transitive cleanup this enables.

update: alternative PR (which is better imo) now available #2176

@XN137
Copy link
Contributor Author

XN137 commented Jul 28, 2025

closing as the alternative PR got merged

@XN137 XN137 closed this Jul 28, 2025
@github-project-automation github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Jul 28, 2025
@XN137 XN137 deleted the PolarisObjectMapper branch July 28, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants