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

new-log-viewer: Add ClpIrDecoder. #62

Merged
merged 11 commits into from
Sep 1, 2024
Merged

Conversation

junhaoliao
Copy link
Collaborator

@junhaoliao junhaoliao commented Aug 29, 2024

References

new-log-viewer series: #45 #46 #48 #51 #52 #53 #54 #55 #56 #59 #60 #61

Description

  1. Integrate clp-ffi-js as a dependency.
  2. Add ClpIrDecoder.
  3. Since now decoder creations could be asynchronous, move decoder initialization out from LogFileManager's constructor into its factory function.
  4. Call buildIdx() in LogFileManager's constructor to deserialize the whole input file.

Validation performed

  1. Referred to Add support for loading files, decoding them as JSONL, and formatting them using a Logback-like format string. #46 , started dev server at default address http://localhost:3010/ .
  2. Loaded example CLP Stream ("IRv1") file: http://localhost:3010/?filePath=https://yscope.s3.us-east-2.amazonaws.com/sample-logs/yarn-ubuntu-resourcemanager-ip-172-31-17-135.log.1.clp.zst
  3. Validated below functions and observed behaviour matched expectaions:
    1. When loaded without specifying the logEventNum in the app URL, the last page was loaded with the cursor set to the last event.
    2. When loaded with logEventNum in the URL, the first page is shown with the cursor set to the first event.
    3. Within the Monaco editor, tested prevPage, nextPage, firstPage, nextPage with shortcuts, and all worked as expected. (One issue was found when the page switching happens too frequently: it was observed that if nextPage is requested too frequently at a time, we could be seeing pages being loaded in order N (currentPageNum) -> N+1 -> N+2 -> N+1; initial analysis believe this was due to multiple requests getting sent to the service worker on page N which caused concurrency issues. It should be addressed in another PR where posting messages are disabled for certain requests that a response from the server and its corresponding handling completion in the render is expected before another new request can be sent.)
    4. Set page size to 1. Reloaded the app without logEventNum and observed the last page containing only the last event was loaded.
    5. Setting invalid (spaces; i.e., " ") JSONL decoder options does not affect the correct behaviours of the CLP Stream decoder.

@junhaoliao junhaoliao changed the title WIP - Integrate ClpStreamReader from clp-ffi-js and add ClpIrDecoder Integrate ClpStreamReader from clp-ffi-js and add ClpIrDecoder Sep 1, 2024
@junhaoliao junhaoliao changed the title Integrate ClpStreamReader from clp-ffi-js and add ClpIrDecoder new-log-viewer: Add ClpIrDecoder with ClpStreamReader from clp-ffi-js integration. Sep 1, 2024
@junhaoliao junhaoliao marked this pull request as ready for review September 1, 2024 05:48
new-log-viewer/src/typings/decoders.ts Outdated Show resolved Hide resolved
/**
* The end index for specifying full range when the exact number of log events is unknown.
*/
const FULL_RANGE_END_IDX: number = 0;
Copy link
Member

Choose a reason for hiding this comment

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

How about LOG_EVENT_FILE_END_IDX?

new-log-viewer/src/services/LogFileManager.ts Outdated Show resolved Hide resolved
new-log-viewer/src/services/LogFileManager.ts Show resolved Hide resolved
new-log-viewer/src/services/LogFileManager.ts Outdated Show resolved Hide resolved
@kirkrodrigues
Copy link
Member

One issue was found when the page switching happens too frequently

Let's file an issue for this so we don't forget.

junhaoliao and others added 2 commits September 1, 2024 15:12
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

For the PR title, how about:

new-log-viewer: Add ClpIrDecoder.

@junhaoliao
Copy link
Collaborator Author

One issue was found when the page switching happens too frequently

Let's file an issue for this so we don't forget.

Reported in #64

@junhaoliao junhaoliao changed the title new-log-viewer: Add ClpIrDecoder with ClpStreamReader from clp-ffi-js integration. new-log-viewer: Add ClpIrDecoder. Sep 1, 2024
@junhaoliao
Copy link
Collaborator Author

@kirkrodrigues please trigger the merge from your end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants