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

removed illegal partition assign from input.py #673

Closed
wants to merge 8 commits into from
Closed

Conversation

rgnf
Copy link

@rgnf rgnf commented Sep 25, 2024

Die für einen subscription based consumer illegale Operation "assign(topic_partition)" aus _lost_callback entfernt.
Ggf. muss noch irgendwas aufgeräumt werden.
Da diese Partitionen potentiell bereits von einem anderen Consumer bearbeitet werden, passt "batch_finished" nicht. Das könnte zu Chaos bei den Offsets führen.
Ob ein "output_connector._write_backlog" Chaos auf der Output Seite auslösen kann, kann ich nicht beurteilen.

Die für einen subscription based consumer illegale Operation "assign(topic_partition)" aus _lost_callback entfernt.
removed illegal partition assign from input.py
@ekneg54
Copy link
Collaborator

ekneg54 commented Sep 25, 2024

Thank you for your PR. The language in this project is English. So please write only in English.
Please read our Contributing Guidelines https://github.com/fkie-cad/Logprep/blob/main/CONTRIBUTING.md and do what is suggested there to contribute to this project.

Until that happen I revert this PR into a draft.

@ekneg54 ekneg54 marked this pull request as draft September 25, 2024 10:57
@rgnf
Copy link
Author

rgnf commented Sep 25, 2024

For subscription based consumers it is not allowed/iilegal to use "assign(topic_partition)". Therefore I removed the offending code from _lost_callback.
Maybe there is need for additional cleanup.
As the lost partitions could be already used by other consumers, a call to batch_finished could cause chaos.
I do not know if calling output_connector._write_backlog could cause chaos on the output part.

New entry in Bugfix section
@rgnf
Copy link
Author

rgnf commented Sep 25, 2024

Updated changelog provided in PR #674 .
No need to update documenation
No test provided, as it is unknown how to reproduce "lost partition" event for testing.

@ekneg54
Copy link
Collaborator

ekneg54 commented Sep 25, 2024

could you add all means depending this change in this pull request please? It is not necessary to open further PRs.
Please fix the failing unittests and adhere to black formatting in your change. Note that the failing containerbuild is not fixable by you.

@rgnf
Copy link
Author

rgnf commented Sep 25, 2024

Was some fiddeling, but now this PR includes also the CHANGELOG.md.
As the changes were made directly in the Github Web UI, I do not know how to "black format" the changes.
Where can I see which Unit Tests fail as result of the change?
I do not believe that this part of the code is covered in any test. Otherwise existing tests would have pointed to this issue long ago.

@ekneg54
Copy link
Collaborator

ekneg54 commented Sep 25, 2024

i triggered the pipeline again. have a look in the failing unittests and in the code quality job. there you are able to see, what is failing

@rgnf
Copy link
Author

rgnf commented Sep 26, 2024

Done, but I do not understand what issue it is complaining about.
If I understand the report correctly, it wants to replace a blank line with a blank line. Can the reporting configured in a way, to show "non printable characters" like newline, carriage return, tab, blanks?
I guess, the Github editor fooled me somehow by unintentionally identation.

@rgnf
Copy link
Author

rgnf commented Sep 26, 2024

Removed identation on blank line.

@rgnf
Copy link
Author

rgnf commented Sep 26, 2024

Now the linter complains about code I did not touch.

@ekneg54
Copy link
Collaborator

ekneg54 commented Sep 28, 2024

close because will be addressed by #678

@ekneg54 ekneg54 closed this Sep 28, 2024
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