Skip to content

[pip][design] PIP-280: Refactor CLI for measurement units (time, byte)#20691

Merged
tisonkun merged 4 commits intoapache:masterfrom
JooHyukKim:Use-converer
Jul 12, 2023
Merged

[pip][design] PIP-280: Refactor CLI for measurement units (time, byte)#20691
tisonkun merged 4 commits intoapache:masterfrom
JooHyukKim:Use-converer

Conversation

@JooHyukKim
Copy link
Copy Markdown
Contributor

Motivation

  • Refer to pip-280.md

Documentation

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

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jun 30, 2023
@JooHyukKim JooHyukKim changed the title [pip][design] PIP-280: Refactor CLI Argument Parsing Logic for Measurement Units using JCommander's custom converter [pip][design] PIP-280: Refactor CLI for measurement units (time, byte) Jul 1, 2023
@tisonkun tisonkun self-requested a review July 4, 2023 05:50
Copy link
Copy Markdown
Member

@tisonkun tisonkun 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 prepare this proposal @JooHyukKim!

Although, I don't see that such a change requires PIP since it doesn't change the user-facing interface but improving or refactoring the internal implementation.

Would you share your thoughts why it should be a PIP? Generally a PIP takes long time to proceed and we don't need it unless necessary.

@JooHyukKim
Copy link
Copy Markdown
Contributor Author

Would you share your thoughts why it should be a PIP? Generally a PIP takes long time to proceed and we don't need it unless necessary.

I see, probably that's why PIP was not quite responsive as I expected. I thought about it, too. Whether this was PIP-sort of thing. As pip introduction mentions, specifically 🔗 at this part, large code refactoring may need PIP also.

Copy link
Copy Markdown
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

OK. Let's give it a try.

I support this proposal and it's clear to be understood. I'll continue on the mailing list and see how we proceed the discussion and vote process.

Comment thread pip/pip-280.md Outdated
Co-authored-by: tison <wander4096@gmail.com>
Comment thread pip/pip-280.md
Copy link
Copy Markdown
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM (non-binding)

@mattisonchao
Copy link
Copy Markdown
Member

+1, (binding)

Since you are working on the CLI part. How about using GraalVM and Picocli for our CLI to improve start time and reduce memory? :)

@tisonkun
Copy link
Copy Markdown
Member

tisonkun commented Jul 4, 2023

@mattisonchao I have experience on build an CLI with GraalVM and PicoCLI - https://github.com/korandoru/hawkeye

But I believe it is far more complex than this proposal wants to do. So we should evaluate it in another round -

  1. GraalVM requires some configs on reflection and our tools have a complex dependency so it could be quite challenging to do it.
  2. PicoCLI is, generally, a different tool to JCommander. I'd recommend a new project to use PicoCLI as in my personal bias it's well designed. But we already heavily use JCommander so even I ever think of that, I ended up with giving up.

I'll appreciate it if any contributor can prototyping and share their findings but notice here that to migrate an existing project is quite complex (think of migrate from Maven to Gradle for Pulsar, or as it happened to BK and later reverted). And it should be apart from the scope of this proposal.

@mattisonchao
Copy link
Copy Markdown
Member

mattisonchao commented Jul 4, 2023

Aha, Thanks for your professional explanation. My comment is just a brainstorm and not related to this PIP. :)
/cc @tisonkun

@BewareMyPower
Copy link
Copy Markdown
Contributor

BewareMyPower commented Jul 7, 2023

Good proposal. I think we need a formal vote email for it.

@JooHyukKim
Copy link
Copy Markdown
Contributor Author

Good proposal. I think we need a formal vote email for it.

@BewareMyPower Right, thank you for letting me know. I was wondering what to do next.

@tisonkun
Copy link
Copy Markdown
Member

tisonkun commented Jul 7, 2023

I formerly think approval is vote, but I read the document that:

Once some consensus is reached, there will be a vote to formally approve the proposal. The vote will be held on the dev@pulsar.apache.org mailing list, by sending a message using subject [VOTE] PIP-xxx: {PIP TITLE}. Make sure to include a link to the PIP PR in the body of the message. Make sure to update the PIP with a link to the vote. You can obtain it from Apache Pony Mail. Everyone is welcome to vote on the proposal, though only the vote of the PMC members will be considered binding. It is required to have a lazy majority of at least 3 binding +1s votes. The vote should stay open for at least 48 hours.

Comment thread pip/pip-280.md
## Links

- Mailing List discussion thread: https://lists.apache.org/thread/b77bfnjlt62w7zywcs8tqklvyokpykok
- Mailing List voting thread: https://lists.apache.org/thread/0r3bh0h7f86g2x9odvrd1fp2gwddq904
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added voting thread. Participation (again...) much appreciated! 🙏🏼
Thanks in advance 🙂
cc/ @mattisonchao @tisonkun @gaoran10 @BewareMyPower

@tisonkun
Copy link
Copy Markdown
Member

Merging...

The vote thread is concluded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli doc-not-needed Your PR changes do not impact docs type/PIP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants