You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2023/01/02 08:58:50 UTC
[GitHub] [airflow] stamixthereal opened a new pull request, #28675: MongoHook optimization
stamixthereal opened a new pull request, #28675:
URL: https://github.com/apache/airflow/pull/28675
Create additional method create_uri, optimized context manager's methods, add unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1063201208
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +85,25 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def _create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ login = self.connection.login
+ password = self.connection.password
+ if login is not None and password is not None:
+ login = login.encode()
+ password = password.encode()
Review Comment:
How does it not work fine? An example would be nice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1063212936
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +85,25 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def _create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ login = self.connection.login
+ password = self.connection.password
+ if login is not None and password is not None:
+ login = login.encode()
+ password = password.encode()
+ creds = f"{quote_plus(login)}:{quote_plus(password)}@"
+ else:
+ creds = ""
+ port = f":{self.connection.port}" if self.connection.port else ""
+ return f"{scheme}://{creds}{self.connection.host}{port}/{self.connection.schema}"
Review Comment:
Implemented `urlunsplit` for creating uri
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #28675:
URL: https://github.com/apache/airflow/pull/28675
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062154743
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -74,12 +68,13 @@ def __exit__(
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
- if self.client is not None:
- self.close_conn()
+ if self.client:
+ self.client.close()
+ self.client = None
def get_conn(self) -> MongoClient:
"""Fetches PyMongo Client"""
- if self.client is not None:
+ if self.client:
Review Comment:
In general, `if self.client` will be faster than `if self.client is not None`. This is because the `is not` operator requires evaluating the object on the right hand side, while the `if` statement only checks the truthiness of the object on the left-hand side.
The interpreter only needs to check the truthiness of `self.client`. This means that `if self.client` will be faster if `self.client` is often `True`, while `if self.client is not None` will be faster `if self.client` is often None.
It's worth noting that the difference in performance will likely be very small, and in most cases, it is more important to write code that is easy to read and understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on PR #28675:
URL: https://github.com/apache/airflow/pull/28675#issuecomment-1370750525
@potiuk @uranusjr Can you guys please review it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062340786
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -74,12 +68,13 @@ def __exit__(
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
- if self.client is not None:
- self.close_conn()
+ if self.client:
+ self.client.close()
+ self.client = None
def get_conn(self) -> MongoClient:
"""Fetches PyMongo Client"""
- if self.client is not None:
+ if self.client:
Review Comment:
That's why I will changes this to @uranusjr way :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062340073
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -74,12 +68,13 @@ def __exit__(
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
- if self.client is not None:
- self.close_conn()
+ if self.client:
+ self.client.close()
+ self.client = None
def get_conn(self) -> MongoClient:
"""Fetches PyMongo Client"""
- if self.client is not None:
+ if self.client:
Review Comment:
I understand your desire to avoid conflicts and to work efficiently with the maintainer. It is important to approach discussions with a constructive attitude and to focus on finding solutions rather than trying to "win" the argument. If the maintainer has a preference for using one approach over the other, it may be more efficient to simply follow their preference, especially if the difference in approaches does not have a significant impact on the code. It is also important to consider the perspective of the maintainer and to be open to their suggestions and feedback. Ultimately, the goal should be to find a solution that works best for the project, rather than trying to prove one's own point of view.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1063131169
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +85,25 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def _create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ login = self.connection.login
+ password = self.connection.password
+ if login is not None and password is not None:
+ login = login.encode()
+ password = password.encode()
Review Comment:
I don’t think these are needed? `quote_plus` works with strings just fine, from my understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062156579
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -74,12 +68,13 @@ def __exit__(
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
- if self.client is not None:
- self.close_conn()
+ if self.client:
+ self.client.close()
+ self.client = None
def get_conn(self) -> MongoClient:
"""Fetches PyMongo Client"""
- if self.client is not None:
+ if self.client:
Review Comment:
That’s not how performance works. This is not a good venue to go into details, and the difference is indeed small. I’d prefer this PR to not change the code unnecessarily.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062144019
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +85,25 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def create_uri(self) -> str:
Review Comment:
```suggestion
def _create_uri(self) -> str:
```
This doesn’t need to be public, from what I can tell?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062144203
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -74,12 +68,13 @@ def __exit__(
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
- if self.client is not None:
- self.close_conn()
+ if self.client:
+ self.client.close()
+ self.client = None
def get_conn(self) -> MongoClient:
"""Fetches PyMongo Client"""
- if self.client is not None:
+ if self.client:
Review Comment:
This is actually (ever so slightly) slower, for what it is worth.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1061613717
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +84,19 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ creds = f"{self.connection.login}:{self.connection.password}@" if self.connection.login else ""
+ port = f":{self.connection.port}" if self.connection.port else ""
+ return f"{scheme}://{creds}{self.connection.host}{port}/{self.connection.schema}"
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1061600064
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +84,19 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ creds = f"{self.connection.login}:{self.connection.password}@" if self.connection.login else ""
+ port = f":{self.connection.port}" if self.connection.port else ""
+ return f"{scheme}://{creds}{self.connection.host}{port}/{self.connection.schema}"
Review Comment:
Absolutely agree with you, @Taragolis! It is generally a good practice to URL encode sensitive information such as login and password, especially when it is included in a URI. This can help prevent potential vulnerabilities caused by parsing the URI in an unexpected way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062336344
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -74,12 +68,13 @@ def __exit__(
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
- if self.client is not None:
- self.close_conn()
+ if self.client:
+ self.client.close()
+ self.client = None
def get_conn(self) -> MongoClient:
"""Fetches PyMongo Client"""
- if self.client is not None:
+ if self.client:
Review Comment:
No opinion. I think it's just a matter of taste. We use both aproaches across our code. There are some cases where using is None matters, here it does not.
I also think it's not worth arguing about and if a maintainer wants it to change, i'd just change it. It's far more constructive and takes far less time to comply with such request that to argue and especially to involve others to be "judges". It does not matter (in this case) if you or other person is right or not. Your goal is to merge the request, not to "win" the discussion.
Really - pick your fights wisely, otherwise you will loose too much energy (of yours and others) so that when there are things that matter, you will not have neither energy or other's willingness to listen when it really matters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1063211122
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +85,25 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def _create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ login = self.connection.login
+ password = self.connection.password
+ if login is not None and password is not None:
+ login = login.encode()
+ password = password.encode()
Review Comment:
Sorry for confusing, It works fine with `quote_plus`. I forgot, that I had the encoding errors only with `quote` function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1063190941
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +85,25 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def _create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ login = self.connection.login
+ password = self.connection.password
+ if login is not None and password is not None:
+ login = login.encode()
+ password = password.encode()
Review Comment:
Unfortunately, it doesn't work fine with the normal strings, that's why I've implemented string encoding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1061553456
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +84,19 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ creds = f"{self.connection.login}:{self.connection.password}@" if self.connection.login else ""
+ port = f":{self.connection.port}" if self.connection.port else ""
+ return f"{scheme}://{creds}{self.connection.host}{port}/{self.connection.schema}"
Review Comment:
This might be not a main idea of this PR but also would be nice if we url-encode login and password:
https://pymongo.readthedocs.io/en/stable/api/pymongo/mongo_client.html#pymongo.mongo_client.MongoClient
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062167244
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -74,12 +68,13 @@ def __exit__(
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
- if self.client is not None:
- self.close_conn()
+ if self.client:
+ self.client.close()
+ self.client = None
def get_conn(self) -> MongoClient:
"""Fetches PyMongo Client"""
- if self.client is not None:
+ if self.client:
Review Comment:
@potiuk What do you think here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1063130165
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +85,25 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def _create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ login = self.connection.login
+ password = self.connection.password
+ if login is not None and password is not None:
+ login = login.encode()
+ password = password.encode()
+ creds = f"{quote_plus(login)}:{quote_plus(password)}@"
+ else:
+ creds = ""
+ port = f":{self.connection.port}" if self.connection.port else ""
+ return f"{scheme}://{creds}{self.connection.host}{port}/{self.connection.schema}"
Review Comment:
Would it be a good idea to use `urlunsplit` instead of string-joining?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1062336344
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -74,12 +68,13 @@ def __exit__(
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
- if self.client is not None:
- self.close_conn()
+ if self.client:
+ self.client.close()
+ self.client = None
def get_conn(self) -> MongoClient:
"""Fetches PyMongo Client"""
- if self.client is not None:
+ if self.client:
Review Comment:
No opinion. I think it's just a matter of taste. We use both aproaches across our code. There are some cases where using is None matters, here it does not.
I also think it's not worth arguing about and if a maintainer wants it to change, i'd just change it. It's far more constructive and takes far less time to comply with such request that to argue and especially to involve others to be "judges". It does not matter (in this case) if you or other person is right or not. Your goal is to merge the request, not to "win" the discussion.
Really - pick your fights wisely, otherwise you will loose too much energy (of yours and others) otherwise when there are things that matter, you will not have neither energy nor other's willingness to listen when it really matters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stamixthereal commented on a diff in pull request #28675: MongoHook optimization
Posted by GitBox <gi...@apache.org>.
stamixthereal commented on code in PR #28675:
URL: https://github.com/apache/airflow/pull/28675#discussion_r1063191447
##########
airflow/providers/mongo/hooks/mongo.py:
##########
@@ -90,15 +85,25 @@ def get_conn(self) -> MongoClient:
options.update({"ssl_cert_reqs": CERT_NONE})
self.client = MongoClient(self.uri, **options)
-
return self.client
- def close_conn(self) -> None:
- """Closes connection"""
- client = self.client
- if client is not None:
- client.close()
- self.client = None
+ def _create_uri(self) -> str:
+ """
+ Create URI string from the given credentials.
+ :return: URI string.
+ """
+ srv = self.extras.pop("srv", False)
+ scheme = "mongodb+srv" if srv else "mongodb"
+ login = self.connection.login
+ password = self.connection.password
+ if login is not None and password is not None:
+ login = login.encode()
+ password = password.encode()
+ creds = f"{quote_plus(login)}:{quote_plus(password)}@"
+ else:
+ creds = ""
+ port = f":{self.connection.port}" if self.connection.port else ""
+ return f"{scheme}://{creds}{self.connection.host}{port}/{self.connection.schema}"
Review Comment:
Agree with you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org