You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/05/24 15:48:39 UTC
[GitHub] [couchdb-ibrowse] noahshaw11 opened a new pull request #6: Fdb dbcore#690 change now/0 to timestamp/0
noahshaw11 opened a new pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6
Change the deprecated `now/0` to `timestamp/0` to eliminate deprecation warnings during building.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r643450507
##########
File path: src/ibrowse_http_client.erl
##########
@@ -463,9 +463,14 @@ accumulate_response(Data, #state{reply_buffer = RepBuf,
State#state{reply_buffer = RepBuf_1}
end.
+strictly_monotonic_timestamp() ->
+ {A, B, _} = erlang:timestamp(),
+ Unique = erlang:unique_integer([positive, monotonic]),
+ {A, B, Unique}.
Review comment:
To keep the microseconds in the format, in case the VM is restarted within one second, we could make the A be the unix seconds, C be microseconds then the unique value.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] noahshaw11 commented on pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#issuecomment-855490765
PR was merged upstream: https://github.com/cmullaparthi/ibrowse/pull/173
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r643450507
##########
File path: src/ibrowse_http_client.erl
##########
@@ -463,9 +463,14 @@ accumulate_response(Data, #state{reply_buffer = RepBuf,
State#state{reply_buffer = RepBuf_1}
end.
+strictly_monotonic_timestamp() ->
+ {A, B, _} = erlang:timestamp(),
+ Unique = erlang:unique_integer([positive, monotonic]),
+ {A, B, Unique}.
Review comment:
To keep the microseconds in the format, in case the VM is restarted within one second, we could make the A be the unix seconds, C be microseconds then the unique value.
Maybe it can be something like `{A * 1000000 + B, C, Unique}
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] noahshaw11 commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638125382
##########
File path: src/ibrowse_http_client.erl
##########
@@ -1870,7 +1870,7 @@ cancel_timer(Ref, {eat_message, Msg}) ->
end.
make_req_id() ->
- now().
+ timestamp().
Review comment:
Actually, it is not strictly monotonically increasing. I believe the best (and easiest) is to generate a unique integer and structure the data as you suggested.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638100198
##########
File path: src/ibrowse_http_client.erl
##########
@@ -465,7 +465,7 @@ accumulate_response(Data, #state{reply_buffer = RepBuf,
make_tmp_filename(true) ->
DownloadDir = ibrowse:get_config_value(download_dir, filename:absname("./")),
- {A,B,C} = now(),
+ {A,B,C} = timestamp(),
Review comment:
`erlang:timestamp()` is right but it's not auto-imported I think?
```
1> erlang:timestamp().
{1621,873425,508305}
2> timestamp().
** exception error: undefined shell command timestamp/0
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] noahshaw11 commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638115290
##########
File path: src/ibrowse_http_client.erl
##########
@@ -1870,7 +1870,7 @@ cancel_timer(Ref, {eat_message, Msg}) ->
end.
make_req_id() ->
- now().
+ timestamp().
Review comment:
How about something like https://erlang.org/doc/man/erlang.html#monotonic_time-0?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638984836
##########
File path: src/ibrowse.erl
##########
@@ -159,7 +159,7 @@ stop() ->
%% headerName() = string()
%% headerValue() = string()
%% response() = {ok, Status, ResponseHeaders, ResponseBody} | {ibrowse_req_id, req_id() } | {error, Reason}
-%% req_id() = term()
+%% req_id() = {MegaSecs::integer >= 0, Secs::integer >= 0, Unique::integer >= 0}
Review comment:
Let's keep this unspecified as a term. Yeah some users might have relied on it being a timestamp or might have matched {_, _, }, or adding some log parsing based on it. But we don't necessarily want to promise this to be a format in the future.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638984836
##########
File path: src/ibrowse.erl
##########
@@ -159,7 +159,7 @@ stop() ->
%% headerName() = string()
%% headerValue() = string()
%% response() = {ok, Status, ResponseHeaders, ResponseBody} | {ibrowse_req_id, req_id() } | {error, Reason}
-%% req_id() = term()
+%% req_id() = {MegaSecs::integer >= 0, Secs::integer >= 0, Unique::integer >= 0}
Review comment:
Let's keep this unspecified as a 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638091546
##########
File path: src/ibrowse_http_client.erl
##########
@@ -465,7 +465,7 @@ accumulate_response(Data, #state{reply_buffer = RepBuf,
make_tmp_filename(true) ->
DownloadDir = ibrowse:get_config_value(download_dir, filename:absname("./")),
- {A,B,C} = now(),
+ {A,B,C} = timestamp(),
Review comment:
I think it would be `os:timestamp()` not just `timestamp()`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] noahshaw11 closed pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
noahshaw11 closed pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638210107
##########
File path: src/ibrowse_http_client.erl
##########
@@ -465,7 +465,7 @@ accumulate_response(Data, #state{reply_buffer = RepBuf,
make_tmp_filename(true) ->
DownloadDir = ibrowse:get_config_value(download_dir, filename:absname("./")),
- {A,B,C} = now(),
+ {A,B,C} = {erlang:unique_integer([positive, monotonic]), 0, 0},
Review comment:
Good call. Here it also expects it to be strictly monotonic. But seeing that this creates a file made me think of another case when the erlang VM is restarted. In that case the counter will be reset to 0 and so filenames might collide. I think we might want to keep at least some connection to wall clock time here.
Perhaps something like:
```
strictly_monotonic_timestamp() ->
{A, B, C} = erlang:timestamp(),
Unique = erlang:unique_integer([positive, monotonic]),
{A, B, C + Unique}.
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] noahshaw11 commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638095535
##########
File path: src/ibrowse_http_client.erl
##########
@@ -465,7 +465,7 @@ accumulate_response(Data, #state{reply_buffer = RepBuf,
make_tmp_filename(true) ->
DownloadDir = ibrowse:get_config_value(download_dir, filename:absname("./")),
- {A,B,C} = now(),
+ {A,B,C} = timestamp(),
Review comment:
I am using this one https://erlang.org/doc/man/erlang.html#timestamp-0 which is the recommended replacement for `now()` according to https://erlang.org/doc/apps/erts/time_correction.html#new-time-api.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] noahshaw11 commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638102803
##########
File path: src/ibrowse_http_client.erl
##########
@@ -465,7 +465,7 @@ accumulate_response(Data, #state{reply_buffer = RepBuf,
make_tmp_filename(true) ->
DownloadDir = ibrowse:get_config_value(download_dir, filename:absname("./")),
- {A,B,C} = now(),
+ {A,B,C} = timestamp(),
Review comment:
Interesting, I need to look into what is auto-imported. I figured `erlang` would be imported automatically. Fixing 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638210107
##########
File path: src/ibrowse_http_client.erl
##########
@@ -465,7 +465,7 @@ accumulate_response(Data, #state{reply_buffer = RepBuf,
make_tmp_filename(true) ->
DownloadDir = ibrowse:get_config_value(download_dir, filename:absname("./")),
- {A,B,C} = now(),
+ {A,B,C} = {erlang:unique_integer([positive, monotonic]), 0, 0},
Review comment:
Good call. Here it also expects it to be strictly monotonic. But seeing that this creates a file made think of another case when the erlang VM is restarted. In that case the counter will be reset to 0 and so filename might collide. I think we might want to keep at least some connection to wall clock time here.
Perhaps something like:
```
strictly_monotonic_timestamp() ->
{A, B, C} = erlang:timestamp(),
Unique = erlang:unique_integer([positive, monotonic]),
{A, B, C + Unique}.
```
##########
File path: src/ibrowse_http_client.erl
##########
@@ -1870,7 +1870,7 @@ cancel_timer(Ref, {eat_message, Msg}) ->
end.
make_req_id() ->
- now().
+ {erlang:unique_integer([positive, monotonic]), 0, 0}.
Review comment:
I saw at least once request ID is sent in a [header](https://github.com/apache/couchdb-ibrowse/blob/3e0e5fffdb63f79d7a55ea38d384fdf4fbede6cf/src/ibrowse_http_client.erl#L970) and could potentially be logged. And since unique integer counter is reset to 0 on every VM restart the behavior might change. So I think we could use the same scheme as for the filename (pull it into a helper function) and could re-use the same logic.
This might still introduce a minor incompatibility if users ever introspected the value of `ReqId` and expected it to be a proper timestamp. It doesn't seem like ibrowse documents `ReqId` to be a `timestamp()` and the type spec in `browse.erl` just says it's a generic [term()](https://github.com/apache/couchdb-ibrowse/blob/master/src/ibrowse.erl#L162). But if you feel like could add it as a comment somewhere in the code or just in the commit message.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] nickva commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638111904
##########
File path: src/ibrowse_http_client.erl
##########
@@ -1870,7 +1870,7 @@ cancel_timer(Ref, {eat_message, Msg}) ->
end.
make_req_id() ->
- now().
+ timestamp().
Review comment:
This one is a bit tricky. `now()` is a strictly monotonically increasing function, it guarantees that each call will give a value larger than the previous while `os:timestamp()` doesn't guarantee that. In most cases, when used as timestamp it doesn't matter but here if it's used to uniquely identify requests, it might change the behavior and possibly create two requests with identical request IDs
We might have to look at the advice in https://erlang.org/doc/apps/erts/time_correction.html#Time_Warp_Modes how to deal with it. Perhaps we'd call `erlang:unique_integer([monotonic])` but we'd also want to keep the shape of the request ID variable in case client code matches on `{_, _, _}`. Maybe something like `{erlang:unique_integer([monotonic, positive]), 0, 0}`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-ibrowse] noahshaw11 commented on a change in pull request #6: Change now/0 to timestamp/0
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #6:
URL: https://github.com/apache/couchdb-ibrowse/pull/6#discussion_r638115290
##########
File path: src/ibrowse_http_client.erl
##########
@@ -1870,7 +1870,7 @@ cancel_timer(Ref, {eat_message, Msg}) ->
end.
make_req_id() ->
- now().
+ timestamp().
Review comment:
How about something like https://erlang.org/doc/man/erlang.html#monotonic_time-0? It would have to be converted to a timestamp I believe.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org