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

[WIP] remove json4s from linkis #4368

Merged
merged 11 commits into from Apr 13, 2023

Conversation

GuoPhilipse
Copy link
Member

@GuoPhilipse GuoPhilipse commented Mar 14, 2023

What is the purpose of the change

use jackson to replace json4s, we can reduce the linkis dependency

Related issues/PRs

Related issues: #4186
Related pr:#4368

Brief change log

  • replace json4s with jackson in linkis

Checklist

  • I have read the Contributing Guidelines on pull requests.
  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the Linkis mailing list first)
  • If this is a code change: I have written unit tests to fully verify the new behavior.

@peacewong
Copy link
Contributor

@jackxu2011 @rarexixi please help review

)
def getYarnResource(resource: Option[Any]) = resource.map(r => {
val value =
JsonUtils.jackson.readValue(r.asInstanceOf[YarnResource].toJson, classOf[Map[String, Any]])
Copy link
Contributor

Choose a reason for hiding this comment

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

is the json is useless, just cast the resource to YarnResource?

Copy link
Member Author

Choose a reason for hiding this comment

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

have given up this method, reduce json parse times.

@@ -298,9 +297,9 @@ class DefaultEngineCreateService
resource,
timeout
) match {
case AvailableResource(ticketId) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to return Json here?

Copy link
Member Author

Choose a reason for hiding this comment

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

seems it is used for getting resource [resourceTicketId, resource] info if we got enough resource, or we will throw not enough resource.

    val emNode = choseNode.get.asInstanceOf[EMNode]
    // 4. 请求资源
    val (resourceTicketId, resource) =
      requestResource(engineCreateRequest, labelFilter.choseEngineLabel(labelList), emNode, timeout)

    // 5. 封装engineBuildRequest对象,并发送给EM进行执行
    val engineBuildRequest = EngineConnBuildRequestImpl(
      resourceTicketId,
      labelFilter.choseEngineLabel(labelList),
      resource,
      EngineConnCreationDescImpl(
        engineCreateRequest.getCreateService,
        engineCreateRequest.getDescription,
        engineCreateRequest.getProperties
      )
    )

0d
val absoluteCapacity = JsonPath.read(r, "$.absoluteCapacity")

if (absoluteCapacity.isInstanceOf[BigDecimal]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The judgment here is not assigned a value in the end. Is it necessary to judge BigDecimal here?

Copy link
Member Author

Choose a reason for hiding this comment

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

have changed to assign the value while judging datatype.

val app = ctx.read("$.apps.app")

val queueValue = ctx.read("$.apps.app.queue").asInstanceOf[String]
val stateValue = ctx.read("$.apps.app.state").asInstanceOf[String]
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it should iterate

}

def serializeResource(resource: Resource): String = {
write(resource)
JsonUtils.jackson.writeValueAsString(resource, classOf[Map[String, Any]])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to specify the type?

Copy link
Contributor

@peacewong peacewong left a comment

Choose a reason for hiding this comment

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

LGTM.

peacewong
peacewong previously approved these changes Mar 23, 2023
Copy link
Contributor

@peacewong peacewong left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@jackxu2011 jackxu2011 left a comment

Choose a reason for hiding this comment

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

LGTM

@GuoPhilipse GuoPhilipse changed the title [feat] remove json4s from linkis [WIP] remove json4s from linkis Apr 12, 2023
@jackxu2011 jackxu2011 merged commit 3702d91 into apache:dev-1.4.0 Apr 13, 2023
8 of 10 checks passed
@GuoPhilipse GuoPhilipse deleted the removejson4sfromcg branch July 2, 2023 09:44
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.

None yet

4 participants