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

[Doc] Describe default ack timeout and update grammar in ConsumerBuilder javadoc #14084

Merged
merged 2 commits into from
Jul 8, 2022
Merged

[Doc] Describe default ack timeout and update grammar in ConsumerBuilder javadoc #14084

merged 2 commits into from
Jul 8, 2022

Conversation

liry
Copy link
Contributor

@liry liry commented Feb 1, 2022 •
edited by Anonymitaet
Loading

Motivation

When reading the javadoc of ackTimeout, user gets information, that default is 0, but this is not true, when dead letters are enabled.
Also update javadoc grammar in ConsumerBuilder.

Modifications

Only javadoc extension.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

  • no-need-doc

  • doc

    Javadoc extension.

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

@liry:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

@liry:Thanks for providing doc info!

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Feb 1, 2022
@liry liry removed their assignment Feb 1, 2022
momo-jun
momo-jun requested changes Feb 18, 2022
View reviewed changes
Copy link
Contributor

@momo-jun momo-jun left a comment

Choose a reason for hiding this comment

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

Hi @liry, thanks for your contribution to docs! I suggest a minor change in the writing style. PTAL.

@liry
Copy link
Contributor Author

liry commented Feb 18, 2022

Hi @liry, thanks for your contribution to docs! I suggest a minor change in the writing style. PTAL.

Hi @momo-jun, thanks for suggestion. Updated. PTAL.

@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-not-needed Your PR changes do not impact docs labels Feb 18, 2022
@Anonymitaet
Copy link
Member

Anonymitaet commented Feb 18, 2022 •
edited
Loading

@liry please label this PR w/ doc
Details: https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0/edit#bookmark=id.5d66olk7l4oz

@liry liry force-pushed the patch-1 branch 2 times, most recently from ca7b187 to 108847e Compare February 22, 2022 07:29
@liry
Copy link
Contributor Author

liry commented Feb 22, 2022 •
edited
Loading

@liry please label this PR w/ doc Details: https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0/edit#bookmark=id.5d66olk7l4oz

@Anonymitaet done

eolivelli
eolivelli reviewed Feb 22, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Thanks for this useful contribution

Can you please update the title and the description? The patch seems broader than what you describe.

@liry liry changed the title [Doc] Describe default ack timeout in javadoc [Doc] Describe default ack timeout and update grammar in ConsumerBuilder javadoc Feb 23, 2022
@liry
Copy link
Contributor Author

liry commented Feb 23, 2022

Thanks for this useful contribution

Can you please update the title and the description? The patch seems broader than what you describe.

@eolivelli done

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

eolivelli
eolivelli approved these changes Mar 26, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

@eolivelli
Copy link
Contributor

@Anonymitaet maybe you want to take a look

@Anonymitaet
Copy link
Member

/pulsarbot run-failure-checks

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@liry
Copy link
Contributor Author

liry commented May 25, 2022

@Anonymitaet is there any problem with this PR?

@Anonymitaet
Copy link
Member

@liry tests do not pass.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Jun 26, 2022
@liry
Copy link
Contributor Author

liry commented Jun 27, 2022

@Anonymitaet are you sure, it's caused by this change? It's only javadoc change and I don't see any failed check in this PR (actually they are waiting for something).

@Anonymitaet
Copy link
Member

@liry I mean this PR can proceed only after the checks are passed. You might need to merge master.

@github-actions github-actions bot removed the Stale label Jun 28, 2022
@liry
[Doc] Update javadoc grammar
1988aa3 
Write in the simple present tense as much as possible if you are covering facts that were, are, and forever shall be true.
@liry
Copy link
Contributor Author

liry commented Jun 28, 2022

@Anonymitaet I've rebased it on top of the current master, but checks are still not running. Isn't this the cause: "First-time contributors need a maintainer to approve running workflows."?

@Anonymitaet
Copy link
Member

@liry I've approved to run the checks

@Anonymitaet
Copy link
Member

/pulsarbot run-failure-checks

@liry
Copy link
Contributor Author

liry commented Jul 4, 2022 •
edited
Loading

@Anonymitaet I don't understand, how my javadoc only change could cause org.apache.pulsar.client.api.v1.V1_ProducerConsumerTest ► testActiveAndInActiveConsumerEntryCacheBehavior to fail ... seems to me more like unstable test.

@github-actions github-actions bot added doc-label-missing and removed doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. labels Jul 5, 2022
@github-actions
Copy link

github-actions bot commented Jul 5, 2022

@liry Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

@github-actions github-actions bot added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-label-missing labels Jul 5, 2022
@dave2wave
Copy link
Member

Thank you for your contribution.

@dave2wave dave2wave merged commit 86ab67f into apache:master Jul 8, 2022
zymap pushed a commit to zymap/pulsar that referenced this pull request Jul 11, 2022
@liry @zymap
[Doc] Describe default ack timeout and update grammar in ConsumerBuil…
1f9d818 
…der javadoc (apache#14084)

* [Doc] Describe default ack timeout in javadoc

* [Doc] Update javadoc grammar

Write in the simple present tense as much as possible if you are covering facts that were, are, and forever shall be true.
wuxuanqicn pushed a commit to wuxuanqicn/pulsar that referenced this pull request Jul 14, 2022
@liry @weimob-wuxuanqi
[Doc] Describe default ack timeout and update grammar in ConsumerBuil…
754439c 
…der javadoc (apache#14084)

* [Doc] Describe default ack timeout in javadoc

* [Doc] Update javadoc grammar

Write in the simple present tense as much as possible if you are covering facts that were, are, and forever shall be true.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@momo-jun momo-jun momo-jun requested changes

@eolivelli eolivelli eolivelli approved these changes

@RobertIndie RobertIndie RobertIndie approved these changes

@Anonymitaet Anonymitaet Anonymitaet approved these changes

Assignees

@liry liry

Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@liry @Anonymitaet @eolivelli @dave2wave @RobertIndie @momo-jun