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

[fix][fn] change async function time get #23820

Closed

Conversation

walkinggo
Copy link

see #23811

Main Issue: see #23705

Motivation

In the realm of asynchronous processing, precise timing and performance metrics are essential for effective monitoring and optimization. Apache Pulsar Functions, as a distributed compute platform, relies heavily on asynchronous operations to process and transform data streams. However, the current implementation lacks a robust mechanism for accurately capturing and reporting the execution times of these asynchronous functions.

The primary motivation for this Pull Request is to address the limitations of the existing asynchronous function execution time tracking system. By centralizing the start time recording and improving the consistency of execution time data, we aim to provide users with a more reliable and comprehensive view of function performance.

Modifications

This Pull Request introduces several modifications to the Apache Pulsar Functions project aimed at improving the accuracy and reliability of asynchronous function execution time statistics. The primary goal is to enhance monitoring and analysis capabilities for function performance. Here’s a breakdown of the key changes:

  1. Removal of processTimeStart Method: The processTimeStart method in ComponentStatsManager has been removed. This method was previously used to record the start time of asynchronous function execution. The start time is now recorded in the JavaExecutionResult object, providing a more centralized and consistent approach.

  2. Modification of JavaInstanceRunnable: The run method in JavaInstanceRunnable has been updated to reflect the removal of processTimeStart. The stats.processTimeStart() call has been deleted, and the stats.processTimeEnd() method now accepts the start time as a parameter to calculate the total execution time.

  3. Update to FunctionStatsManager: The processTimeEnd method in FunctionStatsManager has been modified to remove the processTimeStart member variable and accept the start time as a parameter. This allows for accurate calculation of the function’s execution time.

  4. Changes to SinkStatsManager and SourceStatsManager: The processTimeEnd methods in SinkStatsManager and SourceStatsManager have been updated to accept the start time as a parameter. However, since these classes do not record processing time, the method bodies remain empty.

  5. Addition of startTime in JavaExecutionResult: The JavaExecutionResult class now includes a startTime member variable to store the start time of asynchronous function execution. This allows for accurate calculation of execution time within the handleResult method of JavaInstanceRunnable.

  6. Modification of AsyncFuncRequest: The AsyncFuncRequest class in JavaInstance now includes a result member variable of type JavaExecutionResult. This change ensures that the processAsyncResultsInInputOrder method uses the existing JavaExecutionResult object instead of creating a new one, maintaining consistency and avoiding duplication.

  7. Use of Same ExecutionResult in Non-asyncPreserveInputOrder Mode: In scenarios where asyncPreserveInputOrderForOutputMessages is disabled, the same executionResult object is now used to avoid unnecessary object creation and potential issues with result assignment.

  8. Fix for Result and Exception Handling: Two patches address potential bugs related to result and exception handling in JavaInstance. The processAsyncResultsInInputOrder method now ensures that the result and userException fields of JavaExecutionResult are properly set, improving the reliability of the execution result.

  9. Addition of Test Case: A new test case, testAsyncFunctionTime, has been added to verify the accuracy of asynchronous function execution time recording and calculation. This test ensures that the start time recorded in JavaExecutionResult is within an acceptable range of the actual start time.

Documentation

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

Matching PR in forked repository

PR in forked repository:

@walkinggo
Copy link
Author

I have set the code formatting in IDEA according to Pulsar's settings, but there are still some extra spaces appearing.

@lhotari
Copy link
Member

lhotari commented Jan 7, 2025

I have set the code formatting in IDEA according to Pulsar's settings, but there are still some extra spaces appearing.

@walkinggo When making changes, it's better not to reformat complete source code files since the original files might not be properly formatted. This applies to many files in Pulsar Functions.

@lhotari
Copy link
Member

lhotari commented Jan 7, 2025

I have set the code formatting in IDEA according to Pulsar's settings, but there are still some extra spaces appearing.

@walkinggo When making changes, it's better not to reformat complete source code files since the original files might not be properly formatted. This applies to many files in Pulsar Functions.

It's not a mistake to reformat the files, it just makes the PR diff more verbose. Usually we avoid reformatting complete files when working on the Pulsar source code.

@lhotari
Copy link
Member

lhotari commented Jan 7, 2025

@walkinggo Please rename the title of this PR since the current title "change async function time get" is not understandable. You can use LLMs to review and provide suggestions for more understandable language. I shared some tips in #23806 (comment) about this.

@walkinggo walkinggo force-pushed the change-async-function-time-get branch from 0e0390a to 0344726 Compare January 7, 2025 16:54
@walkinggo walkinggo closed this Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants