Skip to content

MINOR Moved tiered storage API classes from clients module to a new storage-api module.#10489

Merged
junrao merged 1 commit intoapache:trunkfrom
satishd:remote-storage-api
Apr 7, 2021
Merged

MINOR Moved tiered storage API classes from clients module to a new storage-api module.#10489
junrao merged 1 commit intoapache:trunkfrom
satishd:remote-storage-api

Conversation

@satishd
Copy link
Copy Markdown
Member

@satishd satishd commented Apr 6, 2021

MINOR Moved tiered storage API classes from clients module to a new storage-api module.
Created storage and storage-api modules. All the remote storage API classes are moved to storage-api module. All the remote storage implementation classes will be added to storage module.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@satishd
Copy link
Copy Markdown
Member Author

satishd commented Apr 6, 2021

@junrao Raised this PR as you suggested earlier about moving client classes to a separate module. I created storage-api submodule and moved all the remote storage API classes into that module.

…torage-api wmodule.

Created storage and storage-api modules
Copy link
Copy Markdown
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@satishd : Thanks for the PR. LGTM

@junrao junrao merged commit 6e1723b into apache:trunk Apr 7, 2021
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Apr 9, 2021

Thanks. Will we use separate packages for the api vs implementation? We should avoid split packages for the reasons mentioned here: https://www.logicbig.com/tutorials/core-java-tutorial/modules/split-packages.html

@satishd
Copy link
Copy Markdown
Member Author

satishd commented Apr 9, 2021

Thanks @ijuma for bringing this up.

Discussed earlier having api as the suffix for the API related classes. New package name in storage:api module becomes org.apache.kafka.server.log.remote.storage.api.
cc @junrao

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Apr 11, 2021

One option is to use log.remote.storage as the public api package and add internals for the non public. That's similar to what we do for the clients api. We need to verify that this works fine with the module system (I haven't checked in detail if subpackages in separate modules would cause issues, but I'd hope not).

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Apr 11, 2021

Also cc @kowshik

@kowshik
Copy link
Copy Markdown
Contributor

kowshik commented Apr 12, 2021

I just looked at the clients code under kafka/clients/. As an example to @ijuma's suggestion, I see the interface: org.apache.kafka.common.KafkaFuture and its implementation in: org.apache.kafka.common.internals.KafkaFutureImpl. This type of separation looks good too.

@ijuma Does the existing approach in this PR have any disadvantages, or the suggestion to use .internals has more to do with being consistent with how we do it in clients already? I read the article linked above, but I wasn't sure if it'd apply to us here, but I could be wrong.

cc @satishd @junrao

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Apr 12, 2021

This PR has the same package in two separate jars. That is what's called "split packages" in the article above.

@satishd
Copy link
Copy Markdown
Member Author

satishd commented Apr 12, 2021

One option is to use log.remote.storage as the public api package and add internals for the non public. That's similar to what we do for the clients api. We need to verify that this works fine with the module system (I haven't checked in detail if subpackages in separate modules would cause issues, but I'd hope not).

@ijuma I am fine with this approach. It looks like we plan to have this convention for all the modules across the repo.

@junrao
Copy link
Copy Markdown
Contributor

junrao commented Apr 17, 2021

Yet another option is to have a new server-api module. We can move the following server side apis that are currently in the client module to the new module.
include "/org/apache/kafka/server/authorizer/*"
include "
/org/apache/kafka/server/policy/"
include "**/org/apache/kafka/server/quota/
"

If so, we can move the storage api there too.

satishd added a commit to satishd/kafka that referenced this pull request May 31, 2021
…torage-api wmodule. (apache#10489)

Moved tiered storage API classes from clients module to a new storage-api module.
Created storage and storage-api modules. All the remote storage API classes are moved to storage-api module. All the remote storage implementation classes will be added to storage module.

Reviewers: Jun Rao <junrao@gmail.com>
kamalcph pushed a commit to satishd/kafka that referenced this pull request Jun 21, 2021
…torage-api wmodule. (apache#10489)

Moved tiered storage API classes from clients module to a new storage-api module.
Created storage and storage-api modules. All the remote storage API classes are moved to storage-api module. All the remote storage implementation classes will be added to storage module.

Reviewers: Jun Rao <junrao@gmail.com>
satishd added a commit to satishd/kafka that referenced this pull request Aug 12, 2021
…torage-api wmodule. (apache#10489)

Summary:
Moved tiered storage API classes from clients module to a new storage-api module.
Created storage and storage-api modules. All the remote storage API classes are moved to storage-api module. All the remote storage implementation classes will be added to storage module.

apache-reviewers: Jun Rao <junrao@gmail.com>
(cherry picked from commit 6e1723b)

Reviewers: #ldap_kafka_admins, kchandraprakash

Reviewed By: #ldap_kafka_admins, kchandraprakash

JIRA Issues: DKAFC-868

Differential Revision: https://code.uberinternal.com/D6303189
divijvaidya pushed a commit to Hangleton/kafka that referenced this pull request Apr 28, 2022
…torage-api wmodule. (apache#10489)

Summary:
Moved tiered storage API classes from clients module to a new storage-api module.
Created storage and storage-api modules. All the remote storage API classes are moved to storage-api module. All the remote storage implementation classes will be added to storage module.

apache-reviewers: Jun Rao <junrao@gmail.com>
(cherry picked from commit 6e1723b)

Reviewers: #ldap_kafka_admins, kchandraprakash

Reviewed By: #ldap_kafka_admins, kchandraprakash

JIRA Issues: DKAFC-868

Differential Revision: https://code.uberinternal.com/D6303189
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.

4 participants