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 Failing Logging capture #82

Merged
merged 4 commits into from
Jan 20, 2024
Merged

Fix Failing Logging capture #82

merged 4 commits into from
Jan 20, 2024

Conversation

pomadchin
Copy link
Collaborator

@pomadchin pomadchin commented Jan 20, 2024

This PR removes the log collection stream and forwards all PDAL logs to stderr (via stdlog). This much more useful in the context of the pipeline and unhandled exceptions, where the log stream could be collected into a stream only after the entire pipeline execution (which is not guaranteed).

Pipeline constructor signature is also changed, and now no longer accepts LogLevels as integers.

About the nature of a bug:

It looks like the log stream was collected (freed) / reinitialized, and the nature of this behavior is still unknown, I spent a couple of evening on it and could not find a workaround / viable theory. It may depend on how a particular pipeline step is executed because not all the pipelines cause the log failure. However, I feel like the current approach is much more informative and useful.

Connects #79

@WhiteSte could you check this PR on your input? I tried and it seems 👍

@pomadchin pomadchin added the bug Something isn't working label Jan 20, 2024
@WhiteSte
Copy link
Contributor

@WhiteSte could you check this PR on your input? I tried and it seems 👍

Sure, how can i do? Should i go inside your commit, build the natives and test with the new ones?

@pomadchin
Copy link
Collaborator Author

pomadchin commented Jan 20, 2024

@WhiteSte just pull down the PR, build it locally like before and try the same pipeline that's been failing, if that's possible!

@WhiteSte
Copy link
Contributor

WhiteSte commented Jan 20, 2024

Sure i can test that with my local files in few minutes. I had a different bug coming from inside the use of a docker image, but this docker is built using directly the maven release and there for me is really more difficult to do. But i will check it up next days

@WhiteSte
Copy link
Contributor

Is it possible that before that it wasn't possible to stream the log in real time from pdal and now it is like that?
Some time ago i built a Runnable LogCollector that each 1 seconds was calling pipeline.getLog() and printing it to console, because as far as i know from java was impossible to stream the pdal output. But now even without this Collector java is streaming pdal output, this is great it makes the code even simpler.

Btw bad news: pipeline.getLog() crashes now , but the logLevel >3 seems to be fixed at all

@pomadchin
Copy link
Collaborator Author

@WhiteSte there is no getLog method anymore!

@WhiteSte
Copy link
Contributor

WhiteSte commented Jan 20, 2024

Oh sorry i updated just the native libs, not the core jar! So is fixed!
Do you confirm that now the log is streamed automatically? So i can erase my collector

@pomadchin
Copy link
Collaborator Author

Do you confirm that now the log is streamed automatically?

Yes, it is streamed into stderr, looks smth like:

[info] running Main
(javapipeline readers.las Debug) Ignoring setSpatialReference attempt: an override was set
(javapipeline Debug) Executing pipeline in standard mode.
(javapipeline filters.relaxationdartthrowing Debug) Input point cloud has fewer points 1769 than requested output size of 100000. Terminating.
.... // smth else

@WhiteSte
Copy link
Contributor

exactly that's my log now

(javapipeline readers.las Debug) Ignoring setSpatialReference attempt: an override was set
(javapipeline Debug) Executing pipeline in standard mode.
(javapipeline filters.relaxationdartthrowing Debug) Retaining 25000 of 30038 points (83.2279%)
(javapipeline filters.litree Debug) Classifying points for tree 1 (|Ui| = 25000)
...

@WhiteSte
Copy link
Contributor

@WhiteSte there is no getLog method anymore!

Isn't wiser to keep getLog just returning a fake string like "This method is deprecated and will be removed in next version"? So people using new version won't get unexpected crashes

@pomadchin
Copy link
Collaborator Author

I plan to cut a release with a note that it's a breaking change and methods were not functioning; that fits our release scheme (patch version can be breaking).

There are not that many users of these JNI bindings, and I'd prefer not to keep broken things in the codebase.

Unexpected crashes are possible only if users build libraries themsevles and / or experiment with not matching native / jar versions. Normally it should be a compile time error.

@pomadchin
Copy link
Collaborator Author

pomadchin commented Jan 20, 2024

I can include patch version into the native binaries build so it's more strict about native libraries it can pickup, but that can be too agressive.

@WhiteSte
Copy link
Contributor

I can include patch version into the native binaries build so it's more strict about native libraries it can pickup, but that can be too agressive.

yeah maybe it is

@pomadchin pomadchin merged commit 0827c56 into main Jan 20, 2024
4 checks passed
@pomadchin pomadchin deleted the fix/logging branch January 20, 2024 22:05
@pomadchin
Copy link
Collaborator Author

@WhiteSte published as 2.6.0+9-37b3e4ef-SNAPSHOT for now!

@WhiteSte
Copy link
Contributor

where? I don't see it in maven central

@pomadchin
Copy link
Collaborator Author

pomadchin commented Jan 21, 2024

@WhiteSte the link is clickable, it on maven central snapshots (aka oss sonatype snapshots) https://oss.sonatype.org/content/repositories/snapshots/

@pomadchin
Copy link
Collaborator Author

The badges in the README.md are not fake -- kind of having an informative nature of location & versions published; all of them are clickable.

image

@WhiteSte
Copy link
Contributor

got it thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants