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

[INLONG-2161][Feature] Manager support getClusterConfig #2182

Closed
wants to merge 29 commits into from

Conversation

vernedeng
Copy link
Contributor

@vernedeng vernedeng commented Jan 18, 2022

Title Name: [INLONG-2161][Feature] Manager support getClusterConfig

#2161

Motivation

getClusterConfig is the interface that Sort Stand-alone acquire id params and sink params from.
Id params is the params of upstream data store such as the topic of kafka and pulsar which Sort Stand-alone consume from.
While sink params is the dispatch params of Sort Stand-alone Sink, which determine which kafka , pulsar or hive cluster the produced data send to. hence it's something like kafka zklist, brokerlist, pulsar serviceUrl, token..

Here is the example that the config be like.

image

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either 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:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

  • Does this pull request introduce a new feature? yes
  • If yes, how is the feature documented? JavaDocs
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@dockerzhang dockerzhang changed the title Feature/inlong 2161 Manager support getClusterConfig [INLONG-2161][Feature] Manager support getClusterConfig Jan 18, 2022
@gosonzhang
Copy link
Contributor

@ImVan, Why are there so many code submissions?

From the feature description, this modified code should be very small, is there no rebase when submitting the code?

Please rebase the code first, and then submit it, thanks!

@vernedeng vernedeng closed this Jan 18, 2022
@vernedeng vernedeng reopened this Jan 18, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2022

Codecov Report

Merging #2182 (e91c026) into master (58a3f51) will increase coverage by 0.33%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2182      +/-   ##
============================================
+ Coverage     12.22%   12.55%   +0.33%     
- Complexity     1154     1201      +47     
============================================
  Files           413      417       +4     
  Lines         35243    35735     +492     
  Branches       5544     5590      +46     
============================================
+ Hits           4308     4487     +179     
- Misses        30173    30448     +275     
- Partials        762      800      +38     
Impacted Files Coverage Δ
...inlong/tubemq/manager/service/TaskServiceImpl.java 3.65% <0.00%> (-4.88%) ⬇️
.../java/org/apache/flume/sink/tubemq/TubemqSink.java 51.42% <0.00%> (-4.00%) ⬇️
...ong/tubemq/manager/service/ClusterServiceImpl.java 50.00% <0.00%> (-2.95%) ⬇️
...ng/tubemq/server/master/metrics/MasterMetrics.java 90.00% <0.00%> (-2.50%) ⬇️
...emq/server/broker/metrics/BrokerMetricsHolder.java 69.69% <0.00%> (-1.74%) ⬇️
.../tubemq/server/common/utils/WebParameterUtils.java 17.94% <0.00%> (-0.75%) ⬇️
...ng/tubemq/server/broker/metrics/BrokerMetrics.java 97.29% <0.00%> (-0.75%) ⬇️
.../tubemq/client/producer/SimpleMessageProducer.java 12.50% <0.00%> (-0.57%) ⬇️
...emq/server/master/metrics/MasterMetricsHolder.java 86.30% <0.00%> (-0.55%) ⬇️
.../producer/qltystats/DefaultBrokerRcvQltyStats.java 44.14% <0.00%> (-0.40%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58a3f51...e91c026. Read the comment docs.

@vernedeng
Copy link
Contributor Author

vernedeng commented Jan 18, 2022

@gosonzhang
It's a new interface that Sort Stand-alone acquire cluster config including id params and sink params.
Id params is the params of upstream data store such as the topic of kafka and pulsar which Sort Stand-alone consume from.
While sink params is the dispatch params of Sort Stand-alone Sink, which determine which kafka , pulsar or hive cluster the produced data send to. hence it's something like kafka zklist, brokerlist, pulsar serviceUrl, token.
Due to the diversity of upstream data store type and downstrem sink type, there must be many tables to maintain those params of different type of sources and sinks.
Most of files are auto generated by Mybatis code generator, such as those xxxMapper.java and xxxMapper.xml
What we need to care about are those xxxxServiceImpl.java , espacially SortClusterConfigServiceImpl.java which is the core logic that how to generate a valid ClusterConfig

@wardlican
Copy link
Contributor

wardlican commented Jan 19, 2022

image

"result: true" and "errorCode" are not reasonable in the protocol. It is suggested to change them to: "msg" and "code". use return code to judge whether it is successful or not.

where id = #{id,jdbcType=INTEGER}
</delete>
<insert id="insert" parameterType="org.apache.inlong.manager.dao.entity.TaskIdParamsKafkaEntity">
insert into task_id_params_kafka (id, parent_name, topic
Copy link
Contributor

Choose a reason for hiding this comment

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

need to sort out the format here

where id = #{id,jdbcType=INTEGER}
</delete>
<insert id="insert" parameterType="org.apache.inlong.manager.dao.entity.TaskIdParamsPulsarEntity">
insert into task_id_params_pulsar (id, parent_name, topic
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@vernedeng
Copy link
Contributor Author

image

"result: true" and "errorCode" are not reasonable in the protocol. It is suggested to change them to: "msg" and "code". use return code to judge whether it is successful or not.

Thx @wardlican , very nice suggestion. The fields "result" and "errCode" has been replaced with "msg" and "code"

import java.util.Map;

@Data
@ApiModel("sort cluster config")
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid to use lombok plugin for readability. Coder need to add getter/setter code when coder want to find usages. Coder want to check source code, then coder must to install lombok plugin.

@ApiModel("Sort-StandAlone cluster config request")
public class SortStandAloneClusterConfigRequest {

@ApiModelProperty(value = "cluster name")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto


private String taskName;

private static final long serialVersionUID = 1L;
Copy link
Contributor

Choose a reason for hiding this comment

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

incorrect serialVersionUID

Copy link
Contributor

Choose a reason for hiding this comment

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

static variable is before member variable.


private String inlongStreamId;

private static final long serialVersionUID = 1L;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto


private String topic;

private static final long serialVersionUID = 1L;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto


private String brokerList;

private static final long serialVersionUID = 1L;
Copy link
Contributor

Choose a reason for hiding this comment

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

serialVersionUID can be generated by IDE.

enableUpdateByPrimaryKey="true"
enableDeleteByPrimaryKey="true" enableInsert="true"
enableCountByExample="false" enableDeleteByExample="false"
enableSelectByExample="false" enableUpdateByExample="false"/>-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Invalid code need to be removed.

}

// if md5 is the same as last request, return RESP_CODE_NO_UPDATE
String jsonClusterConfig = gson.toJson(clusterConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

gson can not sort parameter name, then md5 maybe not be a stable value.

@vernedeng vernedeng closed this Jan 21, 2022
@vernedeng vernedeng deleted the feature/INLONG-2161 branch January 23, 2022 07:59
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.

None yet

5 participants