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

Add doc for transform functions before sinks #543

Merged
merged 2 commits into from
May 24, 2023

Conversation

cbornet
Copy link
Contributor

@cbornet cbornet commented Apr 26, 2023

Documentation

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

@github-actions github-actions bot added the doc Improvements or additions to documentation label Apr 26, 2023
@@ -466,6 +466,14 @@ For the latest and complete information, see [Pulsar admin docs](pathname:///ref
</Tabs>
````

## Run a Pulsar Function before a sink connector
Copy link
Contributor

Choose a reason for hiding this comment

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

@cbornet I don't quite understand this user task from the heading.
Can you pls also attach the feature PR in this PR's description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. PTAL

docs/io-use.md Outdated Show resolved Hide resolved
@momo-jun
Copy link
Contributor

momo-jun commented May 8, 2023

IIUR, this PR adds docs for apache/pulsar#16740 and apache/pulsar#17445.
CC @Anonymitaet to review and track.

@Anonymitaet
Copy link
Member

@momo-jun OK, thank you!

Copy link
Member

@Anonymitaet Anonymitaet 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 your contribution!

  1. Have you previewed your changes and ensured everything goes as expected?

    If not, please preview your changes locally and attach the screenshots to this PR.

    In this way, you can get your PR merged more quickly.

  2. Does this change apply to multiple doc versions (e.g., 3.0.x)?

    If so, please update them all.

Thank you! 😊

docs/io-use.md Outdated Show resolved Hide resolved
docs/io-use.md Outdated Show resolved Hide resolved
@cbornet
Copy link
Contributor Author

cbornet commented May 9, 2023

Does this change apply to multiple doc versions (e.g., 3.0.x)?
If so, please update them all.

done. PTAL

@Anonymitaet
Copy link
Member

Thanks for your contribution!

  1. Have you previewed your changes and ensured everything goes as expected?
    If not, please preview your changes locally and attach the screenshots to this PR.
    In this way, you can get your PR merged more quickly.
  2. Does this change apply to multiple doc versions (e.g., 3.0.x)?
    If so, please update them all.

Thank you! 😊

Seems that the 1st task is ignored

Copy link
Member

@dave2wave dave2wave left a comment

Choose a reason for hiding this comment

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

When I tried to preview on the PR's branch I encountered yarn issues due to my just previewing main.

% ./preview.sh current
yarn install v1.22.17
info No lockfile found.
[1/5] 🔍  Validating package.json...
error website-next@0.0.0: The engine "node" is incompatible with this module. Expected version "18". Got "19.8.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
yarn run v1.22.17
error website-next@0.0.0: The engine "node" is incompatible with this module. Expected version "18". Got "19.8.1"
error Commands cannot run with an incompatible environment.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@dave2wave dave2wave merged commit bb637e6 into apache:main May 24, 2023
2 checks passed
@tisonkun
Copy link
Member

@dave2wave

error website-next@0.0.0: The engine "node" is incompatible with this module. Expected version "18". Got "19.8.1"

I've update the requirement to >=18 in #576. This PR can be a bit outdated.

@dave2wave
Copy link
Member

@tisonkun When you make changes which then require older PRs to be corrected by others when the "preview" requirement is already quite hard then I think that judgement about the actual change should be applied. This PR is fine. The markdown is fine. If the dialect of markdown is somehow changed then consideration is already required for any PRs in flight.

@tisonkun
Copy link
Member

tisonkun commented May 25, 2023

@dave2wave what do you want to propose here? I don't object to this PR but share the information that a build wart is fixed.

And with #576 you can preview in a containerized environment so that you don't stuck with local binaries settings (except docker :D).

@tisonkun
Copy link
Member

tisonkun commented May 25, 2023

Isn't it general that "the PR is outdated, merge master to see if it fixes your problem"?

We follow this pattern in the main repo for fixing flaky tests.

I agree that I may upgrade all PRs to catch up the newest version but that should not be a requirement. Three possible points here:

  1. Make an online preview - If we can use Vercel, it can be done smoothly (if the contributor want to, actually it's possible like PIP-249 site redesign #560). But we're deploy with Apache server, so it at least requires some development.
  2. CI to verify - Yes, the CI setting can build this PR. You local environment mismatch - it's not a serious problem, right? What if you build master Pulsar with Java 6 or Java 21 EA and encounter problem? Even that the requirement of Node version is fixed.
  3. Ask for keep the PR up-to-date - GitHub has such settings. We can either (1) required it in .asf.yaml (2) softly show the button with help of INFRA (we ever did it for the main repo). But since such situation is seldom, I tend to reduce the burden of settings. Build issues can converge to fixed.

@dave2wave
Copy link
Member

dave2wave commented May 25, 2023

Here is what I did.

  1. Reclone my GitHub Desktop clone of pulsar-site because there was so much change that I was showing the old site2 directory.
  2. I made a change to docusaurus.config.js
  3. Ran ./preview.sh current and confirmed my nonvisual change
  4. Meant to create a PR but it was a commit.
  5. Switched my branch to this PR in GitHub Desktop.
  6. Ran ./preview.sh current and encountered the error.

Well we just had a big change. I think that we clear out the backlog of PRs as much as possible and help the contributors instead of burdening them.

As to your 2 or 3, if there is a way in pulsar-site for github actions to handle any such future dependency changes then I say pick one. If there is a very large PR then we need to take a feature branch approach like is available in apache/www-site

@tisonkun
Copy link
Member

Well we just had a big change. I think that we clear out the backlog of PRs as much as possible and help the contributors instead of burdening them.

You're right. Let me roll up the pending PRs in these two weeks.

As to your 2 or 3

2 is implemented. But yes the build is tested against the branch itself, not the master branch.

@tisonkun
Copy link
Member

@dave2wave all PRs are rolled out now. And the INFRA ticket is filed as https://issues.apache.org/jira/browse/INFRA-24636.

@cbornet cbornet deleted the transform-function branch June 2, 2023 16:58
@visortelle
Copy link
Member

visortelle commented Jun 3, 2023

Make an online preview - If we can use Vercel, it can be done smoothly (if the contributor want to, actually it's possible like #560). But we're deploy with Apache server, so it at least requires some development.

@tisonkun @dave2wave
There is no need for any additional actions by contributor.
Just fork the repo, then make a PR.

Tested it here:
tealtools#7
https://pulsar-site-git-fork-bloombrains-gh1-new-land-359886-teal-tools.vercel.app/

More info:
https://vercel.com/docs/concepts/deployments/git#deploying-forks-of-public-git-repositories

@Anonymitaet
Copy link
Member

@tisonkun for the preview tools, is this possible to use online tools (e.g., Vercel) rather than command-line tools (what we have now)? Not sure if it has resource limitations (free plan) for community use

@tisonkun
Copy link
Member

tisonkun commented Jun 5, 2023

is this possible to use online tools (e.g., Vercel) rather than command-line tools (what we have now)?

It's at contributors choice, we cannot use it upstream because it requires an admin permission to install the vercal app on a specific repo.

You can file an INFRA ticket to see if the INFRA team approve to do so.

@tisonkun
Copy link
Member

tisonkun commented Jun 5, 2023

And be sure that the final deployment is hosted on an Apache Web Server, which can be subtly different from the vercel deployment (e.g., redirections via htaccess is an Apache Web Server feature, it doesn't work on a Vercel deployment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Improvements or additions to documentation
Projects
None yet
6 participants