Skip to content

Improve the TSMgmtUpdateRegister with an optional file name - #10642

Merged
zwoop merged 2 commits into
apache:masterfrom
zwoop:ImproveTSMgmtUpdateRegister
Oct 23, 2023
Merged

zwoop merged 2 commits into
apache:masterfrom
zwoop:ImproveTSMgmtUpdateRegister

Conversation

@zwoop

@zwoop zwoop commented Oct 18, 2023

Copy link
Copy Markdown
Contributor

Providing this filename makes the update registry keep track of file changes automatically. When used, the continuation provided will only be called if changes to the configuration file is detected.

Also, this eliminates the superflous code for calling a named plugin reload. That code is never used, and I don't like having code there "just because", when nothing uses it.

@zwoop zwoop added the TS API label Oct 18, 2023
@zwoop zwoop added this to the 10.0.0 milestone Oct 18, 2023
@zwoop
zwoop force-pushed the ImproveTSMgmtUpdateRegister branch from b024286 to a24f9ec Compare October 18, 2023 21:30
@zwoop
zwoop force-pushed the ImproveTSMgmtUpdateRegister branch 2 times, most recently from 10f26ad to b8bd566 Compare October 18, 2023 22:52

@shukitchan shukitchan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@zwoop
zwoop force-pushed the ImproveTSMgmtUpdateRegister branch 4 times, most recently from 2681761 to 980bed9 Compare October 19, 2023 22:30
@zwoop

zwoop commented Oct 22, 2023

Copy link
Copy Markdown
Contributor Author

[approve ci autest]

1 similar comment
@shukitchan

Copy link
Copy Markdown
Contributor

[approve ci autest]

@shukitchan

Copy link
Copy Markdown
Contributor
  1. Since it is an API change, should we send an email to the mailing list of the change?

  2. The "name" is really useless. So it does not have to be a map. A list will do. Right?. I am ok with the current change unless you want to improve it further.

zwoop added 2 commits October 22, 2023 17:39
Providing this filename makes the update registry keep track
of file changes automatically. When used, the continuation
provided will only be called if changes to the configuration
file is detected.

Also, this eliminates the superflous code for calling a named
plugin reload. That code is never used, and I don't like having
code there "just because", when nothing uses it.
@zwoop
zwoop force-pushed the ImproveTSMgmtUpdateRegister branch from 7abc310 to 4cad488 Compare October 22, 2023 23:57
@zwoop

zwoop commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author
  1. Since it is an API change, should we send an email to the mailing list of the change?

Yeh, I can email, but it doesn't break or modify any existing plugins (because we can now provide defaults in API changes).

  1. The "name" is really useless. So it does not have to be a map. A list will do. Right?. I am ok with the current change unless you want to improve it further.

With # 2, yes, it's useless, but removing it would break compatibility unnecessarily. I'd rather keep it, if not only because it could at least allow for easier debugging later.

@brbzull0 brbzull0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good!
Thanks.

@zwoop
zwoop merged commit dc49274 into apache:master Oct 23, 2023
@zwoop
zwoop deleted the ImproveTSMgmtUpdateRegister branch October 23, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants