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.

Sign up for GitHub

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

Jump to bottom

[feat][ci] check style for all source code #18142

Merged
merged 4 commits into from
Oct 21, 2022
Merged

[feat][ci] check style for all source code #18142

merged 4 commits into from
Oct 21, 2022

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Oct 21, 2022 •
edited
Loading

This closes #18131.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in the forked repository

PR in forked repository: tisonkun#7

@tisonkun tisonkun mentioned this pull request Oct 21, 2022
Closed
2 tasks
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 21, 2022
@tisonkun tisonkun mentioned this pull request Oct 21, 2022
Closed
@RobertIndie
Copy link
Member

Looks good to me. Need to fix all the check styles issues to pass the CI.

@tisonkun
Copy link
Member Author

@RobertIndie all violations fixed. Please help with triggering CI on the main repo.

@lhotari @codelipenghui I hope that this patch can get reviewed timely so that we don't conflict with incoming PRs - it touches a bunch of files.

@tisonkun
fix violations
c72a0df 
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
no snapshot update
631b28a 
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

tisonkun commented Oct 21, 2022 •
edited
Loading

Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check (default-cli) on project pulsar: Execution default-cli of goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check failed: Plugin org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2 or one of its dependencies could not be resolved: org.apache.pulsar:buildtools:jar:2.11.0-SNAPSHOT was not found in https://repository.apache.org/snapshots during a previous attempt. This failure was cached in the local repository and resolution is not reattempted until the update interval of apache.snapshots has elapsed or updates are forced

Dirty environment issue?

@tisonkun
remove mysterious dependency
0eda68e 
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

0eda68e remove mysterious dependency. I don't know whether it means some tricky logics.

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2022 •
edited
Loading

Codecov Report

Merging #18142 (0eda68e) into master (6c65ca0) will increase coverage by 11.05%.
The diff coverage is 30.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #18142       +/-   ##
=============================================
+ Coverage     34.91%   45.97%   +11.05%     
- Complexity     5707    17663    +11956     
=============================================
  Files           607     1574      +967     
  Lines         53396   128545    +75149     
  Branches       5712    14152     +8440     
=============================================
+ Hits          18644    59095    +40451     
- Misses        32119    63335    +31216     
- Partials       2633     6115     +3482
Flag Coverage Δ
unittests 45.97% <30.00%> (+11.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../main/java/org/apache/pulsar/PulsarStandalone.java 0.00% <0.00%> (ø)
.../apache/pulsar/broker/admin/impl/ClustersBase.java 76.66% <0.00%> (+67.62%) ⬆️
...pache/pulsar/broker/admin/impl/NamespacesBase.java 63.25% <ø> (+48.72%) ⬆️
.../pulsar/broker/service/AbstractBaseDispatcher.java 50.92% <0.00%> (+5.06%) ⬆️
...che/pulsar/broker/service/BacklogQuotaManager.java 12.39% <0.00%> (+2.91%) ⬆️
.../pulsar/broker/service/BrokerServiceException.java 38.88% <0.00%> (+13.88%) ⬆️
...ava/org/apache/pulsar/broker/service/Consumer.java 71.09% <0.00%> (+9.09%) ⬆️
...ava/org/apache/pulsar/broker/service/Producer.java 62.42% <0.00%> (+2.78%) ⬆️
...pulsar/broker/service/PulsarCommandSenderImpl.java 74.35% <0.00%> (+7.69%) ⬆️
...va/org/apache/pulsar/broker/service/ServerCnx.java 48.54% <0.00%> (+3.20%) ⬆️
... and 1168 more

coderzc
coderzc reviewed Oct 21, 2022
View reviewed changes
pulsar-io/jdbc/core/src/main/java/org/apache/pulsar/io/jdbc/JdbcAbstractSink.java
Comment on lines +336 to +341
private void internalFlushBatch(
Deque<Record<T>> swapList,
PreparedStatement currentBatch,
int count,
long start
) throws SQLException {
Copy link
Member

Choose a reason for hiding this comment

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

It seems to be repeated with #18146.

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderzc I think we should proceed with this patch since it adds a CI task to prevent regression.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it's OK I do some rebases.

@tisonkun tisonkun mentioned this pull request Oct 21, 2022
Closed
4 tasks
@tisonkun
Copy link
Member Author

/pulsarbot run-failure-checks

@nicoloboschi nicoloboschi merged commit ae7722b into apache:master Oct 21, 2022
@nicoloboschi nicoloboschi mentioned this pull request Oct 21, 2022
Closed
1 task
@tisonkun tisonkun deleted the patch-3 branch October 21, 2022 09:42
@tisonkun tisonkun mentioned this pull request Nov 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderzc coderzc coderzc approved these changes

@Technoboy- Technoboy- Technoboy- approved these changes

@RobertIndie RobertIndie RobertIndie approved these changes

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

Assignees
No one assigned
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CI for checking code style of pulsar connectors
8 participants
@tisonkun @RobertIndie @codecov-commenter @Technoboy- @BewareMyPower @nicoloboschi @poorbarcode @coderzc