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

Configurable data source for offloaded messages #8717

Merged
merged 34 commits into from
Jan 8, 2021

Conversation

Renkai
Copy link
Contributor

@Renkai Renkai commented Nov 26, 2020

Fix issue: #8591

This PR include:

  • API change in command tools
  • Related implementation with tests
  • Related docs in cookbook

By the way log4j dependency is removed for module managed-ledger because now the whole project use log4j2 as the default logger framework.

@Renkai Renkai marked this pull request as ready for review November 26, 2020 07:09
@Renkai
Copy link
Contributor Author

Renkai commented Nov 26, 2020

/pulsarbot run-failure-checks

@gaoran10
Copy link
Contributor

Great work, maybe we could add a configuration like tieredStorageReadPriority at the config file broker.conf or standalone.conf for broker level configuration.

@Renkai
Copy link
Contributor Author

Renkai commented Dec 2, 2020

Great work, maybe we could add a configuration like tieredStorageReadPriority at the config file broker.conf or standalone.conf for broker level configuration.
@gaoran10
Now added to broker.conf

@Renkai
Copy link
Contributor Author

Renkai commented Dec 2, 2020

/pulsarbot run-failure-checks

Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
@Renkai
Copy link
Contributor Author

Renkai commented Dec 3, 2020

/pulsarbot run-failure-checks

zymap and others added 3 commits December 3, 2020 14:36
---

*Motivation*

The pakcage management module build failed by the spotbugs check.
That will block the project build.
Signed-off-by: Renkai <gaelookair@gmail.com>
@codelipenghui codelipenghui added this to the 2.8.0 milestone Dec 3, 2020
@Renkai
Copy link
Contributor Author

Renkai commented Dec 3, 2020

/pulsarbot run-failure-checks

2 similar comments
@Renkai
Copy link
Contributor Author

Renkai commented Dec 3, 2020

/pulsarbot run-failure-checks

@Renkai
Copy link
Contributor Author

Renkai commented Dec 4, 2020

/pulsarbot run-failure-checks

Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
@sijie
Copy link
Member

sijie commented Jan 4, 2021

@hangc0276 Can you review this PR again?

@hangc0276
Copy link
Contributor

@hangc0276 Can you review this PR again?

@sijie done

# Conflicts:
#	managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@Renkai Overall the change looks great. I have left a couple of comments. Please take a look.

if (this.offloadReadPriorityStr != null) {
try {
offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Copy link
Member

Choose a reason for hiding this comment

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

@Renkai Can we avoid using System.out.println?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sijie This code is in a command-line tool, I think stdout is a good choice for users to get information

Copy link
Member

Choose a reason for hiding this comment

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

This is a set command, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sijie I check the code again, I think it's ok to remove println so we can use mute as success information, if the value which user set is an invalid value, the tool will throw a exception. The println was removed.

if (this.offloadReadPriorityStr != null) {
try {
offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Copy link
Member

Choose a reason for hiding this comment

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

@Renkai Can we avoid using System.out.println?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

But this is a set command, no? Why people need this information when setting the offloaded priority?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

Signed-off-by: Renkai <gaelookair@gmail.com>
Signed-off-by: Renkai <gaelookair@gmail.com>
if (this.offloadReadPriorityStr != null) {
try {
offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Copy link
Member

Choose a reason for hiding this comment

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

But this is a set command, no? Why people need this information when setting the offloaded priority?

if (this.offloadReadPriorityStr != null) {
try {
offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Copy link
Member

Choose a reason for hiding this comment

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

This is a set command, no?

Signed-off-by: Renkai <gaelookair@gmail.com>
@Renkai Renkai requested a review from sijie January 6, 2021 08:47
@sijie sijie added area/tieredstorage doc-required Your PR changes impact docs and you will update later. labels Jan 8, 2021
@sijie sijie merged commit 7c09f5c into apache:master Jan 8, 2021
@sijie
Copy link
Member

sijie commented Jan 8, 2021

Great contributions!

codelipenghui pushed a commit that referenced this pull request Jan 31, 2021
Fix issue: #8591

This PR include:
* API change in command tools
* Related implementation with tests
* Related docs in cookbook

By the way  log4j dependency is removed for module `managed-ledger` because now the whole project use log4j2 as the default logger framework.

(cherry picked from commit 7c09f5c)
codelipenghui pushed a commit that referenced this pull request Jan 31, 2021
Fix issue: #8591

This PR include:
* API change in command tools
* Related implementation with tests
* Related docs in cookbook

By the way  log4j dependency is removed for module `managed-ledger` because now the whole project use log4j2 as the default logger framework.

(cherry picked from commit 7c09f5c)
@Anonymitaet
Copy link
Member

@Renkai many thanks for your docs! While site2/docs/cookbooks-tiered-storage.md is deprecated. We will fix it.

@Anonymitaet
Copy link
Member

@Renkai I've submitted #9795 to add docs, could you please help review? Thanks

@Anonymitaet Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Mar 5, 2021
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

7 participants