Skip to content

Conversation

@rekby
Copy link
Member

@rekby rekby commented May 18, 2023

Close #296

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Other information

@rekby rekby requested a review from Valeria1235 May 18, 2023 15:56
ydb/topic.py Outdated
encoder_executor: Optional[concurrent.futures.Executor] = None, # default shared client executor pool
) -> TopicWriter:
args = locals()
args = locals().copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

why you make a copy?

Copy link
Member Author

@rekby rekby May 19, 2023

Choose a reason for hiding this comment

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

because locals() return dynamic dict with infinite recursive field "args" and I can't remove it at next line without get own copy of dict.

self.close()
except ydb.Error:
if exc_val:
raise exc_val
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe raise exc_val from e for keep full context?

Copy link
Member Author

Choose a reason for hiding this comment

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

exc_val may contain some context alredy.

In this line I want preserve old exception if it exist.
exc_val from e will write about e if cause of excl_val and it will difficult to debug.

But the code don't need re-raise exc_val exception at all, i have fixed it, thanks.

@rekby rekby merged commit 49c85b3 into main May 19, 2023
@rekby rekby deleted the 296-fix-write-topic-after-driver-stop branch May 19, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants