You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "jaydoane (via GitHub)" <gi...@apache.org> on 2023/05/08 19:53:02 UTC
[GitHub] [couchdb] jaydoane opened a new pull request, #4581: Otp 26 compatability
jaydoane opened a new pull request, #4581:
URL: https://github.com/apache/couchdb/pull/4581
<!-- Thank you for your contribution!
Please file this form by replacing the Markdown comments
with your text. If a section needs no action - remove it.
Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
of code collaboration. Positive feedback is represented +1 from committers
and negative is a -1. The -1 also means veto, and needs to be addressed
to proceed. Once there are no objections, the PR can be merged by a
CouchDB committer.
See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
## Overview
Build successfully with OTP 26.
<!-- Please give a short brief for the pull request,
what problem it solves or how it makes things better. -->
## Testing recommendations
<!-- Describe how we can test your changes.
Does it provide any behaviour that the end users
could notice? -->
## Related Issues or Pull Requests
<!-- If your changes affect multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] Documentation changes were made in the `src/docs` folder
- [ ] Documentation changes were backported (separated PR) to affected branches
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4581: OTP 26 compatability
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199139839
##########
soak:
##########
@@ -0,0 +1,9 @@
+#!/bin/bash -ex
+
+for run in {1..1000}
Review Comment:
That's a nice script! Our Makefile has [soak-eunit](https://github.com/apache/couchdb/blob/main/Makefile#L203-L205) wonder if that still works and perhaps we could have a `soak-mango` there too (but as a separate PR perhaps).
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a diff in pull request #4581: OTP 26 compatability
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199131583
##########
src/couch_epi/src/couch_epi_util.erl:
##########
@@ -22,7 +22,7 @@ module_version(Module) ->
VSNs.
hash(Term) ->
- <<SigInt:128/integer>> = couch_hash:md5_hash(term_to_binary(Term)),
+ <<SigInt:128/integer>> = couch_hash:md5_hash(term_to_binary(Term, [{minor_version, 1}])),
Review Comment:
Maybe add the same macro into `couch_epi.hrl`?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatability
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199126821
##########
src/couch_log/src/couch_log_trunc_io.erl:
##########
@@ -352,7 +352,7 @@ map_body(Map, Max, #print_options{depth = Depth}) when Max < 4; Depth =:= 0 ->
_ -> {"...", 3}
end;
map_body(Map, Max, Options) ->
- case maps:to_list(Map) of
+ case lists:sort(maps:to_list(Map)) of
Review Comment:
Fixed in https://github.com/apache/couchdb/pull/4581/commits/7408ff8dc6d8771ae09b8640fbeeb424be155674
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatability
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1198365260
##########
src/couch_log/src/couch_log_trunc_io.erl:
##########
@@ -352,7 +352,7 @@ map_body(Map, Max, #print_options{depth = Depth}) when Max < 4; Depth =:= 0 ->
_ -> {"...", 3}
end;
map_body(Map, Max, Options) ->
- case maps:to_list(Map) of
+ case lists:sort(maps:to_list(Map)) of
Review Comment:
I re-enabled it with [a workaround](https://github.com/apache/couchdb/pull/4581/commits/bbda103b304332e2736da30cbbcfe1f03b48683a), but still think it should be fixed.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on pull request #4581: OTP 26 compatibility
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#issuecomment-1555961696
> Minor nit: the title should say "OTP 26 _compatibility_".
Oops, fixed!
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatibility
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199650333
##########
soak:
##########
@@ -0,0 +1,9 @@
+#!/bin/bash -ex
+
+for run in {1..1000}
Review Comment:
Removed in https://github.com/apache/couchdb/pull/4581/commits/9538395451acf72d028c0db2e4cbc9277dcaa589
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jiahuili430 commented on a diff in pull request #4581: OTP 26 compatability
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199110618
##########
src/couch_epi/src/couch_epi_util.erl:
##########
@@ -22,7 +22,7 @@ module_version(Module) ->
VSNs.
hash(Term) ->
- <<SigInt:128/integer>> = couch_hash:md5_hash(term_to_binary(Term)),
+ <<SigInt:128/integer>> = couch_hash:md5_hash(term_to_binary(Term, [{minor_version, 1}])),
Review Comment:
Why not use `?term_to_bin()` here?
```erl
<<SigInt:128/integer>> = couch_hash:md5_hash(?term_to_bin(Term)),
```
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a diff in pull request #4581: OTP 26 compatability
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199132733
##########
src/couch_log/src/couch_log_formatter.erl:
##########
@@ -467,12 +467,14 @@ format_meta(Meta) ->
%% https://www.rfc-editor.org/rfc/rfc5424.html#section-6.3
%% iut="3" eventSource="Application" eventID="1011"
string:join(
- maps:fold(
- fun(K, V, Acc) ->
- [to_str(K, V) | Acc]
- end,
- [],
- Meta
+ lists:sort(
Review Comment:
I don't think we have a requirement to have meta information sorted.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a diff in pull request #4581: OTP 26 compatability
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199167428
##########
src/couch_log/src/couch_log_formatter.erl:
##########
@@ -467,12 +467,14 @@ format_meta(Meta) ->
%% https://www.rfc-editor.org/rfc/rfc5424.html#section-6.3
%% iut="3" eventSource="Application" eventID="1011"
string:join(
- maps:fold(
- fun(K, V, Acc) ->
- [to_str(K, V) | Acc]
- end,
- [],
- Meta
+ lists:sort(
Review Comment:
I think we should fix the test. Here is one suggestion. Define a macro
```
-define(assertOneOff(A, B),
?assertEqual(lists:any(fun(E) -> B == E end, A), B)
).
```
Then provide all combinations
```
?assertOneOff(
[
"[foo=123 baz=\"baz\" bar=\"barStr\"]",
"[foo=123 bar=\"barStr\" baz=\"baz\"]",
"[baz=\"baz\" foo=123 bar=\"barStr\"]",
"[baz=\"baz\" bar=\"barStr\" foo=123]",
"[bar=\"barStr\" foo=123 baz=\"baz\"]",
"[bar=\"barStr\" baz=\"baz\" foo=123]"
],
lists:flatten(Entry#log_entry.msg)
),
```
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] pgj commented on pull request #4581: OTP 26 compatability
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#issuecomment-1555960175
Minor nit: the title should say "OTP 26 _compatibility_".
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatability
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1190180060
##########
src/couch_log/src/couch_log_trunc_io.erl:
##########
@@ -352,7 +352,7 @@ map_body(Map, Max, #print_options{depth = Depth}) when Max < 4; Depth =:= 0 ->
_ -> {"...", 3}
end;
map_body(Map, Max, Options) ->
- case maps:to_list(Map) of
+ case lists:sort(maps:to_list(Map)) of
Review Comment:
This particular fix is for [these tests](https://github.com/apache/couchdb/blob/main/src/couch_log/src/couch_log_trunc_io.erl#L856-L956), which have not been recently changed.
However, [this test](https://github.com/apache/couchdb/blob/main/src/couch_log/test/eunit/couch_log_formatter_test.erl#L35-L44) is recent and I agree it should be fixed. I've disabled it for now.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a diff in pull request #4581: OTP 26 compatability
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1189816053
##########
src/couch_log/src/couch_log_trunc_io.erl:
##########
@@ -352,7 +352,7 @@ map_body(Map, Max, #print_options{depth = Depth}) when Max < 4; Depth =:= 0 ->
_ -> {"...", 3}
end;
map_body(Map, Max, Options) ->
- case maps:to_list(Map) of
+ case lists:sort(maps:to_list(Map)) of
Review Comment:
I think we should update the test instead.
ping @chewbranca
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatability
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199135034
##########
src/couch_log/src/couch_log_formatter.erl:
##########
@@ -467,12 +467,14 @@ format_meta(Meta) ->
%% https://www.rfc-editor.org/rfc/rfc5424.html#section-6.3
%% iut="3" eventSource="Application" eventID="1011"
string:join(
- maps:fold(
- fun(K, V, Acc) ->
- [to_str(K, V) | Acc]
- end,
- [],
- Meta
+ lists:sort(
Review Comment:
[This test as written](https://github.com/apache/couchdb/blob/main/src/couch_log/test/eunit/couch_log_formatter_test.erl#L35-L44) can only pass on both OTP 25 and 26 if we sort the keys here, and then expect them in the sorted order.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a diff in pull request #4581: OTP 26 compatibility
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1202802115
##########
src/couch_log/src/couch_log_formatter.erl:
##########
@@ -467,12 +467,14 @@ format_meta(Meta) ->
%% https://www.rfc-editor.org/rfc/rfc5424.html#section-6.3
%% iut="3" eventSource="Application" eventID="1011"
string:join(
- maps:fold(
- fun(K, V, Acc) ->
- [to_str(K, V) | Acc]
- end,
- [],
- Meta
+ lists:sort(
Review Comment:
I had a concern that sorting the map elements would harm the performance. However given the fact that reports would be small we can have a compromise. Which is every time we introduce a new report type we would try to select shorter names for keys.
I am not going to block this PR on the map order grounds.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatibility
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199657856
##########
src/couch_log/src/couch_log_formatter.erl:
##########
@@ -467,12 +467,14 @@ format_meta(Meta) ->
%% https://www.rfc-editor.org/rfc/rfc5424.html#section-6.3
%% iut="3" eventSource="Application" eventID="1011"
string:join(
- maps:fold(
- fun(K, V, Acc) ->
- [to_str(K, V) | Acc]
- end,
- [],
- Meta
+ lists:sort(
Review Comment:
While I don't love this test, I also don't think making it more complicated really improves it.
IMO those failing tests pointed out actual bugs in our code, which failed to specify the key order of a formatted map, leaving it up to the OTP implementation to decide. Now that the (undocumented) order changed, I think it would be best to sort the keys ourselves, and not continue relying on how OTP decides to order formatted map keys, and possibly keep having to fix the tests going forward.
What are the arguments against determining the formatted key order once and for all?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a diff in pull request #4581: OTP 26 compatability
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199168675
##########
src/couch_log/test/eunit/couch_log_formatter_test.erl:
##########
@@ -38,9 +38,8 @@ format_report_test() ->
bar => "barStr",
baz => baz
}),
- % NOTE: this currently hardcodes the ordering of the keys, however, map
- % key order is not guaranteed and this may break.
- Formatted = "[foo=123 baz=\"baz\" bar=\"barStr\"]",
+ % Rely on `couch_log_formatter:format_meta/1` to sort keys
+ Formatted = "[bar=\"barStr\" baz=\"baz\" foo=123]",
Review Comment:
See my suggestion above
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatability
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199165955
##########
soak:
##########
@@ -0,0 +1,9 @@
+#!/bin/bash -ex
+
+for run in {1..1000}
Review Comment:
> That's a nice script! Our Makefile has [soak-eunit](https://github.com/apache/couchdb/blob/main/Makefile#L203-L205) wonder if that still works and perhaps we could have a `soak-mango` there too (but as a separate PR perhaps).
oops that shouldn't have been added!
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatability
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199123874
##########
src/couch_epi/src/couch_epi_util.erl:
##########
@@ -22,7 +22,7 @@ module_version(Module) ->
VSNs.
hash(Term) ->
- <<SigInt:128/integer>> = couch_hash:md5_hash(term_to_binary(Term)),
+ <<SigInt:128/integer>> = couch_hash:md5_hash(term_to_binary(Term, [{minor_version, 1}])),
Review Comment:
I mentioned in the [individual commit](https://github.com/apache/couchdb/pull/4581/commits/14d8f8f1d1fb67576561f52106a85f929213fa03) that it's not possible to import `couch_db.hrl` into that file.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane merged pull request #4581: OTP 26 compatibility
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane merged PR #4581:
URL: https://github.com/apache/couchdb/pull/4581
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatibility
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199657856
##########
src/couch_log/src/couch_log_formatter.erl:
##########
@@ -467,12 +467,14 @@ format_meta(Meta) ->
%% https://www.rfc-editor.org/rfc/rfc5424.html#section-6.3
%% iut="3" eventSource="Application" eventID="1011"
string:join(
- maps:fold(
- fun(K, V, Acc) ->
- [to_str(K, V) | Acc]
- end,
- [],
- Meta
+ lists:sort(
Review Comment:
While I don't love the failing test that requires this change, I also don't think making it more complicated really improves it.
IMO those failing tests pointed out actual bugs in our code, which failed to specify the key order of a formatted map, leaving it up to the OTP implementation to decide. Now that the (undocumented) order changed, I think it would be best to sort the keys ourselves, and not continue relying on how OTP decides to order formatted map keys, and possibly keep having to fix the tests going forward.
What are the arguments against determining the formatted key order once and for all?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4581: OTP 26 compatibility
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4581:
URL: https://github.com/apache/couchdb/pull/4581#discussion_r1199650386
##########
src/couch_epi/src/couch_epi_util.erl:
##########
@@ -22,7 +22,7 @@ module_version(Module) ->
VSNs.
hash(Term) ->
- <<SigInt:128/integer>> = couch_hash:md5_hash(term_to_binary(Term)),
+ <<SigInt:128/integer>> = couch_hash:md5_hash(term_to_binary(Term, [{minor_version, 1}])),
Review Comment:
Done in https://github.com/apache/couchdb/pull/4581/commits/e5a9bdd4d018695cfddece67e551f97f683fd0b5
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org