[CDRIVER-3087] Allow setting and getting of wtimeoutMS as an int64_t Created: 16/Apr/19 Updated: 28/Oct/23 Resolved: 01/Aug/19 |
|
| Status: | Closed |
| Project: | C Driver |
| Component/s: | libmongoc |
| Affects Version/s: | None |
| Fix Version/s: | 1.15.0 |
| Type: | Improvement | Priority: | Minor - P4 |
| Reporter: | Patrick Freed | Assignee: | Haris Sheikh (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | neweng | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||
| Description |
|
The read/write concern spec defines wtimeoutMS to be a 64 bit integer, but it is implemented as an int32_t in libmongoc. (see mongo_write_concern_(get|set)_wtimeout) For full spec compliance, there should be api for setting and retrieving wtimeoutMS as an int64_t. Currently, drivers that wrap libmongoc have no way of supporting 64-bit wtimeoutMS, so they in turn are also not spec compliant. |
| Comments |
| Comment by Githook User [ 06/Aug/19 ] | |
|
Author: {'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}Message: | |
| Comment by Githook User [ 06/Aug/19 ] | |
|
Author: {'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}Message: Revert " This reverts commit e49f2a3dc6ee3cb5bfb78ca1f96da76e2e7d8865. | |
| Comment by Githook User [ 06/Aug/19 ] | |
|
Author: {'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}Message: | |
| Comment by Githook User [ 01/Aug/19 ] | |
|
Author: {'name': 'Haris Sheikh', 'email': 'harissheikh1998@gmail.com', 'username': 'haris-sheikh'}Message: | |
| Comment by Githook User [ 01/Aug/19 ] | |
|
Author: {'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}Message: Revert " This reverts commit e49f2a3dc6ee3cb5bfb78ca1f96da76e2e7d8865. | |
| Comment by Githook User [ 01/Aug/19 ] | |
|
Author: {'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}Message: | |
| Comment by Jeremy Mikola [ 29/Jul/19 ] | |
Looking at the implementation for mongoc_uri_get_option_as_int32, the fallback (zero in our case) will be returned if the option is not a 32-bit integer. Looking further at mongoc_uri_apply_options and mongoc_uri_bson_append_or_replace_key, I think that URI options are limited to 32-bit integers and that there is no way to express a 64-bit integer. I don't think we need to change any of that, since parsing 64-bit integers would be a new feature and this extends beyond the scope of wtimeoutms. | |
| Comment by Kevin Albertson [ 26/Jul/19 ] | |
|
Regarding wtimeout of > INT32_MAX being ignored, we explicitly check that when parsing integers in the URI and error for out of range: Since there's no known need to set a wtimeout outside of the range of an int32, I don't think there's a reason to support int64 wtimeout in the URI. Plus, there is also a way to set an int64 wtimeout on the client, by using mongoc_client_set_write_concern.
No. But I think that's a fair argument. The 32 bit variants will work just as well for users. And I think not deprecating would be keeping in line with making the least destructive change. I'll comment in the review. | |
| Comment by Haris Sheikh (Inactive) [ 24/Jul/19 ] | |
jmikola by properly ignore a value in that range do you mean that mongoc_uri_get_option_as_int32 should not somehow be able to set a 64-bit integer greater than INT32_MAX? Because I don’t see how that would be possible, but would you like me to add some sort of test to explicitly check that an attempt to provide a value above INT32_MAX will be ignored/raise an error?
I updated this to not enforce the range of a 32-bit integer, and updated the test-case that would have previously viewed this as incorrectly passing. The rest of the test suite seems to still pass (locally and on evergreen), but please let me know if this may have broken something or caused some unintended functionality.
To my understanding these values are in the range of 32-bit integers, so I don’t see it being an issue, but I can update it if we want? And then in regards to deprecations, kevin.albertson previously suggested I should deprecate both the setters and getters to encourage users to switch over to the new _int64 versions, as there is no significant reason not to. However, I don’t have a strong opinion on either option, so I can remove the deprecations on either or leave them as is depending on whatever you think is best. Otherwise I think I fixed all of the issues you raised and have put up the patch for code review. Please take a look though in case I missed anything, and thanks for noticing these issues in the first place! | |
| Comment by Jeremy Mikola [ 17/Jul/19 ] | |
Something that occurred to me while upgrading PHP ( If that sounds viable, then perhaps create_commit_retry_wc can just be modified to use mongoc_write_concern_set_w instead when addressing this ticket. | |
| Comment by Jeremy Mikola [ 17/Jul/19 ] | |
|
While integrating this into PHPC, I came across a few issues that I think warrant re-opening this ticket. Code formattingWas code formatting applied to mongoc-write-concern.c after the change? There are a few cases where it looks like indentation is not aligned correctly (see: https://github.com/mongodb/mongo-c-driver/commit/943adca2630ee7d6e9196c5cd683afda6d8df3de#diff-69e203f1bea1cae2c7307e410778588eR226). Missing documentationThe commit introduced two new methods (mongoc_write_concern_set_wtimeout_int64 and mongoc_write_concern_get_wtimeout_int64), but they are not documented. _mongoc_write_concern_freeze_mongoc_write_concern_freeze still appends a non-zero write_concern->wtimeout value using bson_append_int32. This suggests that any value outside the range of a signed 32-bit integer will be truncated, and likely defeats the purpose of introducing an int64_t setter. I think this should be bson_append_int64. When fixing this, we should add a regression test to demonstrate that a 64-bit wtimeoutms value is properly preserved. create_commit_retry_wcIn create_commit_retry_wc, mongoc_write_concern_get_wtimeout_int64 is used to fetch the value into a local int32_t wtimeout variable. That variable is then passed to mongoc_write_concern_set_wmajority, which expects an int32_t. This seemingly conflits with the spec language quoted within create_commit_retry_wc:
A wtimeout value in the range of a 64-bit integer will end up truncated. This begs the question of whether a new mongoc_write_concern_set_wmajority method should have been created to complement mongoc_write_concern_set_wtimeout_int64. _mongoc_uri_build_write_concernIn _mongoc_uri_build_write_concern, the wtimeoutms variable is obtained with mongoc_uri_get_option_as_int32. I realize that _mongoc_write_concern_new_from_iterWhile mongoc_write_concern_new_from_iter was changed to accept a 64-bit integer _type, it still requires that it not exceed the range of a signed 32-bit integer. Was this the most minimum change to meet Kaitlin's previous request? Combined with _mongoc_write_concern_freeze, I would think that this still prevents Swift users from specifying 64-bit integers (see: SWIFT-395). Test filesIn test-conveniences.c and test-mongoc-read-write-concern.c, test-mongoc-write-concern.c is using BEGIN and END_IGNORE_DEPRECATIONS to call mongoc_write_concern_set_wtimeout. Is this the only remaining call to that method? Is there a particular reason this wasn't changed to use mongoc_write_concern_set_wtimeout_int64? If we wanted test coverage for the original method, I think it'd be preferable to do so within test_write_concern_basic earlier in the file (using the same syntax to ignore deprecations for a block of code). DeprecationsWas it really necessary to deprecate the original functions? There may be an argument for doing so with mongoc_write_concern_get_wtimeout, since it could inadvertently truncate a result, but I think the various setters that accept an int32_t type for wtimeoutms are still fine to use for applications that have no need for larger values (which is most everyone). There may be an argument for leaving mongoc_write_concern_get_wtimeout alone and not deprecated. Since this likely affects to few applications, I think we could get by with some documentation that informs users a 64-bit value will be truncated to the int32_t return type and suggest they use the int64_t getter if that will be a concern. | |
| Comment by Githook User [ 26/Jun/19 ] | |
|
Author: {'name': 'Haris Sheikh', 'email': 'harissheikh@Hariss-MacBook-Pro.local'}Message: | |
| Comment by Githook User [ 25/Jun/19 ] | |
|
Author: {'name': 'Kevin Albertson', 'username': 'kevinAlbs', 'email': 'kevin.albertson@mongodb.com'}Message: Revert " This reverts commit e06044caa14a0ad431209de58460b3ba6a1c3b58. | |
| Comment by Githook User [ 21/Jun/19 ] | |
|
Author: {'name': 'Haris Sheikh', 'email': 'harissheikh@Hariss-MacBook-Pro.local'}Message: | |
| Comment by Kaitlin Mahar [ 03/Jun/19 ] | |
|
Apologies for only bringing this up now that the ticket is in code review, but in some places in the Swift driver we send libmongoc a write concern document embedded in an options document, for example
Currently in those cases, if wtimeout is an Int64, libmongoc gives an invalid write concern error. If it hasn't already been done, it would be very helpful to us if the code for parsing a WriteConcern from a document would also accept an Int64 being used in the document for wtimeout. | |
| Comment by Kevin Albertson [ 23/Apr/19 ] | |
|
Thanks for checking if a spec change was warranted. I can't imagine a use case for a wtimeout of > 49 days, but who knows, maybe wtimeout will have special values in the future. For the sake of spec consistency I suppose there's no harm in adding get|set_wtimeout_int64. | |
| Comment by Patrick Freed [ 22/Apr/19 ] | |
|
So it seems the spec change won't be happening, and that the C driver is exempt from adhering to it (results of SPEC-1282). Is there any possibility an overload of the function that operates on int64_t s is added? | |
| Comment by Patrick Freed [ 22/Apr/19 ] | |
|
We could do the same validation, but it doesn't get around the issue that the spec dictates that our api should accept an int64 that we have no way of actually setting properly. I agree that such a large timeout is unlikely, so I've filed SPEC-1282 to discuss expanding the api to allow 32-bit integers. | |
| Comment by Kevin Albertson [ 22/Apr/19 ] | |
|
PHP already validates that wtimeout is in the range of a 32 bit integer before calling get|set_wtimeout. Can we do the same in swift? Needing a timeout larger than what a 32 bit integer can hold seems very unlikely. This might warrant a spec change to accept both 32/64 bit integers. |