justinmclean commented on PR #4710: URL: https://github.com/apache/iggy/pull/4710#issuecomment-6588702229
Thanks for this. The record fields and value ranges match the Rust sink data producer, the SDK calls match the Python stubs, and the examples job ran it. A few things to change before it goes in: - `random_alnum` calls `secrets.choice` once per character. `secrets` asks the operating system for secure randomness on every call, which is slow when it runs millions of times, and the string generation alone takes about 1.8 s for a full run compared with 0.2 s using `random`. Demo data does not need secure randomness, so please use `random.choices(alphabet, k=length)` here and `random` for the other fields in `random_record`. Add `# noqa: S311` to those lines so the ruff security check accepts them. - The README lists `IGGY_ADDRESS`, `IGGY_USERNAME` and `IGGY_PASSWORD`, but `parse_args` only uses them when no connection string is passed on the command line. Someone who passes one and also sets the variables will not see the variables take effect. Please add a sentence saying the connection string replaces them. - The username and password go into the default connection URL without encoding, so a password containing `@` or `:` fails with a parse error. Wrapping both in `urllib.parse.quote(..., safe="")` fixes it. - The `while` loop with `batches_count` can be `for _ in range(MAX_BATCHES):`, and `random_record` can return a plain dict, which removes the `Record` dataclass and the `asdict` call. The PR description says AI helped and the pre-commit hooks were not run. CONTRIBUTING.md asks for both: please run the `team-review` skill on the branch and install the hooks with `prek install` before you push the next revision. CI skipped the Python lint job for this path, so the hooks are the only lint check it gets. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
