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

Bind processId to construct CodeGenerator #15848

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

ruanwenjun
Copy link
Member

Purpose of the pull request

Right now the CodeGenerateUtils will not bind port, this means if we have multiple process which have the same host name, then these process will generate duplicate code.

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

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

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@mergeable mergeable bot removed the backend label Apr 15, 2024
@qingwli qingwli added the improvement make more easy to user or prompt friendly label Apr 15, 2024
@qingwli qingwli added this to the 3.2.2 milestone Apr 15, 2024
qingwli
qingwli previously approved these changes Apr 15, 2024
Copy link
Member

@qingwli qingwli left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,4 +1,6 @@
/** Copyright 2010-2012 Twitter, Inc.*/
/**
Copy link
Member

Choose a reason for hiding this comment

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

This header looks unusual to me. Is it compatible with Apache 2.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the origin code is copy from twitter, so need to announce it.

Copy link
Contributor

@rickchengx rickchengx left a comment

Choose a reason for hiding this comment

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

Overall LGTM, please fix CI

image

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixDuplicateCode branch 4 times, most recently from b441149 to 5092d32 Compare April 15, 2024 10:55
@ruanwenjun ruanwenjun added the bug Something isn't working label Apr 15, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 59.25926% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 39.59%. Comparing base (f99d3f1) to head (2249d3c).

❗ Current head 2249d3c differs from pull request most recent head 62a7665. Consider uploading reports for the commit 62a7665 to get more accurate results

Files Patch % Lines
...hinscheduler/tools/demo/ProcessDefinitionDemo.java 0.00% 8 Missing ⚠️
...api/service/impl/ProcessDefinitionServiceImpl.java 50.00% 4 Missing ⚠️
...che/dolphinscheduler/api/python/PythonGateway.java 0.00% 3 Missing ⚠️
...lphinscheduler/common/utils/CodeGenerateUtils.java 90.47% 1 Missing and 1 partial ⚠️
...er/api/service/impl/TaskDefinitionServiceImpl.java 66.66% 1 Missing ⚠️
...nscheduler/service/process/ProcessServiceImpl.java 0.00% 1 Missing ⚠️
...ler/tools/datasource/dao/ProcessDefinitionDao.java 0.00% 1 Missing ⚠️
...phinscheduler/tools/datasource/dao/ProjectDao.java 0.00% 1 Missing ⚠️
...ce/upgrader/v200/V200DolphinSchedulerUpgrader.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##                dev   #15848   +/-   ##
=========================================
  Coverage     39.59%   39.59%           
+ Complexity     5017     5011    -6     
=========================================
  Files          1347     1347           
  Lines         45646    45646           
  Branches       4892     4892           
=========================================
+ Hits          18073    18074    +1     
+ Misses        25649    25648    -1     
  Partials       1924     1924           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ruanwenjun ruanwenjun changed the title Bind processId to constructor CodeGenerator Bind processId to construct CodeGenerator Apr 15, 2024
Copy link

sonarcloud bot commented Apr 15, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
48.8% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

Copy link
Member

@qingwli qingwli left a comment

Choose a reason for hiding this comment

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

LGTM

@qingwli qingwli merged commit 27d0563 into apache:dev Apr 16, 2024
59 of 60 checks passed
@ruanwenjun ruanwenjun deleted the dev_wenjun_fixDuplicateCode branch May 31, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend bug Something isn't working improvement make more easy to user or prompt friendly ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants