Skip to content

Remove deepcopy of client._http #3954

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

Merged
merged 3 commits into from
Sep 20, 2017
Merged

Conversation

liyanhui1228
Copy link
Contributor

Fixes #3931.
When GOOGLE_CLOUD_DISABLE_GRPC=true, the logging will have 'AuthorizedSession' object has no attribute 'credentials' exception.
The line http = copy.deepcopy(client._http) is a relic when httplib2 was used. Removing it and directly use the client passed in instead of re-initialize with the deepcopied http object solved the problem.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 13, 2017
@liyanhui1228 liyanhui1228 added the api: logging Issues related to the Cloud Logging API. label Sep 13, 2017
http = copy.deepcopy(client._http)
self.client = client.__class__(
client.project, client._credentials, http)
self.client = client

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@lukesneeringer
Copy link
Contributor

Side note: CODEOWNERS does not seem to work quite right. :-(

@lukesneeringer
Copy link
Contributor

On the merits of this actual PR -- this does look sane to me, but hoping @tseaver or @jonparrott will make sure I am not missing something obvious.

@waprin
Copy link
Contributor

waprin commented Sep 14, 2017

The original deepcopy was a race condition for the async long handler, but it's covered in the system test, being a race condition it didn't always appear but it did frequently enough that if there's any problem with the PR you'd probably know soon. LGTM if jon think it look right.

@liyanhui1228
Copy link
Contributor Author

Thanks @waprin! I have run the system tests for several times locally and it works fine. Though the race condition does not always appear as Bill said. If thread-safety is the only reason for deepcopy, sounds like we can safely remove it. Can we go ahead and merge this?

@liyanhui1228
Copy link
Contributor Author

The system test test_create_sink_pubsub_topic failed with error message from google.cloud.pubsub import client as pubsub_client ImportError: cannot import name client, which seems need to be updated to match the recent pubsub changes. Probably fix it in another PR?

@lukesneeringer
Copy link
Contributor

@liyanhui1228 Go ahead and fix that here. Happy to merge once tests pass.

@liyanhui1228
Copy link
Contributor Author

Ready for merging, system tests passed locally.

tests/system.py::TestLogging::test_create_metric PASSED
tests/system.py::TestLogging::test_create_sink_bigquery_dataset PASSED
tests/system.py::TestLogging::test_create_sink_pubsub_topic PASSED
tests/system.py::TestLogging::test_create_sink_storage_bucket PASSED
tests/system.py::TestLogging::test_list_entry_with_unregistered PASSED
tests/system.py::TestLogging::test_list_metrics PASSED
tests/system.py::TestLogging::test_list_sinks PASSED
tests/system.py::TestLogging::test_log_handler_async _has_entries. Trying again in 1 seconds...
PASSED
tests/system.py::TestLogging::test_log_handler_sync _has_entries. Trying again in 1 seconds...
PASSED
tests/system.py::TestLogging::test_log_root_handler It was the best of times.
_has_entries. Trying again in 1 seconds...
_has_entries. Trying again in 2 seconds...
PASSED
tests/system.py::TestLogging::test_log_struct _has_entries. Trying again in 1 seconds...
PASSED
tests/system.py::TestLogging::test_log_struct_w_metadata _has_entries. Trying again in 1 seconds...
PASSED
tests/system.py::TestLogging::test_log_text _has_entries. Trying again in 1 seconds...
PASSED
tests/system.py::TestLogging::test_log_text_w_metadata _has_entries. Trying again in 1 seconds...
PASSED
tests/system.py::TestLogging::test_log_text_with_resource _has_entries. Trying again in 1 seconds...
PASSED
tests/system.py::TestLogging::test_log_text_with_timestamp _has_entries. Trying again in 1 seconds...
PASSED
tests/system.py::TestLogging::test_reload_metric PASSED
tests/system.py::TestLogging::test_reload_sink PASSED
tests/system.py::TestLogging::test_update_metric PASSED
tests/system.py::TestLogging::test_update_sink PASSED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants