Skip to content

Conversation

@merlimat
Copy link
Contributor

@merlimat merlimat commented Jun 3, 2021

Motivation

Instead of using POJOs types in the Java client-admin API, use interfaces

Modifications

  • Converted more POJOs into interfaces
  • Added builders to construct instances of the interfaces without directly using the implementation classes

Note: for easier reviewing, there are 2 commits in this PR:

  • f14ce73 includes the changes to production code
  • 23b6f74 changes to the unit tests

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Jun 3, 2021
@merlimat merlimat added this to the 2.8.0 milestone Jun 3, 2021
@merlimat merlimat self-assigned this Jun 3, 2021
@merlimat merlimat force-pushed the admin-api-no-deps-constructors branch 3 times, most recently from f7421f5 to 9d0172d Compare June 4, 2021 00:19
@Anonymitaet
Copy link
Member

Hi @merlimat thanks for your contribution. For this PR, do we need to update docs?

@merlimat merlimat force-pushed the admin-api-no-deps-constructors branch from 9d0172d to 4ea3d37 Compare June 4, 2021 16:16
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@hangc0276
Copy link
Contributor

/pulsarbot run-failure-checks

}

static Builder builder() {
return ReflectionUtils.newBuilder("org.apache.pulsar.common.policies.data.TenantInfoImpl");
Copy link
Contributor

Choose a reason for hiding this comment

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

Whether should we put rg.apache.pulsar.common.policies.data.TenantInfoImpl into pulsar-client-admin-api module instead of pulsar-common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for it is the presence of Swagger annotations which cannot be put as dependency of the pulsar-client-admin-api module.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure to use pulsar-client-admin-api dependent project must include pulsar-common dependency. Otherwise, it will throw no such class exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. You won't use pulsar-client-admin-api directly, but through pulsar-client-admin which is a shaded package, or in the context of functions, the -api module will be exposed to the function while the pulsar-client-admin will be kept in the framework class loader.

@codelipenghui codelipenghui merged commit da71ec2 into apache:master Jun 6, 2021
@merlimat merlimat deleted the admin-api-no-deps-constructors branch June 6, 2021 04:39
BewareMyPower added a commit to streamnative/kop that referenced this pull request Jun 8, 2021
### Motivation
apache/pulsar#10818 and apache/pulsar#10774 introduced API changes to `ClusterDataImpl` and `TenantInfoImpl`.

### Modifications
- Fix current code for new interface.
- Remove unused setup code.
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
apache#10818)

### Motivation

Instead of using POJOs types in the Java client-admin API, use interfaces 

### Modifications
 * Converted more POJOs into interfaces
 * Added builders to construct instances of the interfaces without directly using the implementation classes

Note: for easier reviewing, there are 2 commits in this PR: 
 * apache@f14ce73 includes the changes to production code
 * apache@23b6f74 changes to the unit tests
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
apache#10818)

### Motivation

Instead of using POJOs types in the Java client-admin API, use interfaces 

### Modifications
 * Converted more POJOs into interfaces
 * Added builders to construct instances of the interfaces without directly using the implementation classes

Note: for easier reviewing, there are 2 commits in this PR: 
 * apache@f14ce73 includes the changes to production code
 * apache@23b6f74 changes to the unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants