heisenbergs-uncertainty commented on PR #3541: URL: https://github.com/apache/streampipes/pull/3541#issuecomment-2761947259
> Hi @heisenbergs-uncertainty, > > thanks a lot for your PR! The changes are highly welcome. I really appreciate the update to the new API and the additional functionality for the sink. > ### Required Changes > > There are some changes required to ensure that this PR works and that existing instances of the `RestSink` are migrated correclty. > > * **Checkstyle Issues**: There are some checkstyle problems—please check the GitHub Actions logs for details. > > * **Version Update**: Update the sink version to **1** (see line 74). > > * **Migration Script**: A migration script is required to update existing sinks in pipelines. You can refer to [`DataLakeSinkMigrationV2`](https://github.com/apache/streampipes/blob/43573850423475a429f517db6502fbe30d1a25b8/streampipes-extensions/streampipes-sinks-internal-jvm/src/main/java/org/apache/streampipes/sinks/internal/jvm/datalake/migrations/DataLakeSinkMigrationV2.java#L33) for guidance. > > * **Displayed Labels**: Move static property texts to `string.en` in resource directory `org.apache.streampipes.sinks.brokers.jvm.rest`. Use the `Labels.withId` API—see the `URL_KEY` parameter of the `RestSink` before the changes for reference. > > > Please let us know if you need any assistance or if some of the changes I described are not clear. > ### Suggestion > > What do you think about making `MAX_RETRIES` and `RETRY_DELAY_MS` user-configurable with default values? This would allow users to adjust them as needed. > > Thank you again for your PR and we look forward to working with you. Thank you for the suggestions! This is helpful to figure out how to clean this up and get the PR passing. I'll get the version update and migration script done asap. The displayed labels comment makes a lot of sense and answers some of my confusion. I spent a long time looking for why so many components were using "withId" instead of "from". It would be helpful to include this in the "Extending Streampipes" tutorial section on the website. I could have also just missed it while skimming but I did look for a decent period of time and couldn't understand its usage. In regards to the retry delay, I had this same thought and was planning on adding it later on but I will go ahead and include it now! Makes sense to include this in case anybody doesn't want the retry functionality up front. -- 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]
