Personal/mchalain/cleanup openssl

Hello Happy World,
I worked on the port of mbedtls in place of openssl.
My first step was to cleanup the specific code of OpenSSL to move it in only one file and to have an API to use with mbedtls.
Then I join a collection of patches that move the code slowly and minimize the modifications into the sequences.
Some patches are more important than the others:

0009-openssl-store-specific_config-into-srv_socket.patch : this patch is not dangerous but the modification may be use to simplify some other part of the code.

0011-openssl-manage-tlsext_server_name-into-connection.patch : the tlsext_server_name is allocated only for HTTPS connection, but I didn't find configuration to allow to use it.

0012-openssl-move-SSL-configuration-into-new-function.patch : the connection configuration copied all variables. This patch copy the SSL configuration only for a HTTPS connection.

0014-opensll-anonymize-SLL-variables-into-base.h.patch : the certificates variables were copied into each connection, but they were the same for each server_socket. Then this patch copy a pointer to a structure that contains the variables.

Before to continue I would like to discuss about this patches, to be sure that I didn't take the wrong way.
My next step will be to create a mod_openssl but this may introduce some bugs and regressions. Then the cleanup has to be ready before to begin this step.
Best regards, merry Christmas and see you next year,
Marc.

Thanks for the effort!
I had hoped you would see my update to https://redmine.lighttpd.net/boards/4/topics/6988 where I ported mod_openssl from a (diverged) development branch of mine and pushed so that you could see its state.  From a quick glance, it looks like many of your patches have been addressed in the dev branch posted at the link above.
Even if some of the work has been duplicated, I am sure that your changes will be useful to inform me how you were approaching the module and what you need to further developing mbedtls module for lighttpd.  I am interested in your feedback on the dev branch posted in https://redmine.lighttpd.net/boards/4/topics/6988 and what further changes might be needed.  Cheers.
x-ref:
https://redmine.lighttpd.net/boards/4/topics/6988
https://redmine.lighttpd.net/boards/3/topics/7029

Hello,
I'll read your dev/branch and answer quickly.
Thank you
2016-12-31 5:20 GMT+01:00 Glenn Strauss<EMAIL_ADDRESS>
Thanks for the effort!
I had hoped you would see my update to https://redmine.lighttpd.net/
boards/4/topics/6988 where I ported mod_openssl from a (diverged)
development branch of mine and pushed so that you could see its state. From
a quick glance, it looks like many of your patches have been addressed in
the dev branch posted at the link above.
Even if some of the work has been duplicated, I am sure that your changes
will be useful to inform me how you were approaching the module and what
you need to further developing mbedtls module for lighttpd. I am interested
in your feedback on the dev branch posted in https://redmine.lighttpd.net/
boards/4/topics/6988 and what further changes might be needed. Cheers.
x-ref:
https://redmine.lighttpd.net/boards/4/topics/6988
https://redmine.lighttpd.net/boards/3/topics/7029
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/lighttpd/lighttpd1.4/pull/76#issuecomment-269848445,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD9CpGyekmnG_63kc3mfmVSmPQK4Deeaks5rNdf3gaJpZM4LYPjC
.

Hello Glenn,
I read each of your commits and I wrote some remarks and questions. They
are not requests or criticisms, but a beginning for more discussions.
I'm interested to help you to develop some features. And I will begin with
mbedTLS.
acc912f70913
connection_read_cq: why "force_assert(max_bytes == MAX_READ_LIMIT)" ? the
max_bytes parameter is useless.
connection_write_cq: why connection_read_cq inside connections-glue.c and
connection_write_cq inside connections.c ? What's the meaning of "-glue" ?
623f3fcbb0c5
array_insert_unique: free the buffer of tlsext_server_name without check
the value.
tlsext_server_name: free here but not allocated into the mod.
40d3ed3c24e5
config_insert: link ssl support to OpenSSL, but ssl may be supported by
others libraries.
262634ef6f7bd9845de0d
config_insert: unused-slot-moved-to-mod-openssl is a good idea, but for an
embeded server, it's taken memroy for nothing.
mod_openssl_handle_con_accept: retrieves the specific_config of srv_socket
with sidx, but the connection contains the specific_config. What is the
difference ?
connection_init: use the macro's name "CLEAN" to initiliaze, is not a good
idea. I think that a renaming should be interesting.
network_server_init: what is the link between openssl and the modification
of the parameter of this function ? May another commit gives more
explanation ?
show_features: display SSL support for OpenSSL only.
be66471cbcf5a71ad2fc
connection_read_cq: I see that you are agree with my previous question. But
do you known the meaning of "-glue" in this case ?
Best regards,
Marc.
2017-01-02 13:48 GMT+01:00 Marc Chalain<EMAIL_ADDRESS>
Hello,
I'll read your dev/branch and answer quickly.
Thank you
2016-12-31 5:20 GMT+01:00 Glenn Strauss<EMAIL_ADDRESS>
Thanks for the effort!
I had hoped you would see my update to https://redmine.lighttpd.net/b
oards/4/topics/6988 where I ported mod_openssl from a (diverged)
development branch of mine and pushed so that you could see its state. From
a quick glance, it looks like many of your patches have been addressed in
the dev branch posted at the link above.
Even if some of the work has been duplicated, I am sure that your changes
will be useful to inform me how you were approaching the module and what
you need to further developing mbedtls module for lighttpd. I am interested
in your feedback on the dev branch posted in
https://redmine.lighttpd.net/boards/4/topics/6988 and what further
changes might be needed. Cheers.
x-ref:
https://redmine.lighttpd.net/boards/4/topics/6988
https://redmine.lighttpd.net/boards/3/topics/7029
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/lighttpd/lighttpd1.4/pull/76#issuecomment-269848445,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD9CpGyekmnG_63kc3mfmVSmPQK4Deeaks5rNdf3gaJpZM4LYPjC
.

@mchalain:  I have tried to answer your questions below, and I hope the answers are useful to you.
On the other hand, I question the utility of your "code review" comments and remarks.  Maybe you should omit your "code review" comments until you are more familiar with the code base?
Overall, the patches aim to isolate the openssl code into a module, separate from the main server, to allow an alternate module to be written (e.g. mod_mbedtls) which provides similar functionality, also isolated from the main server.

connection_read_cq: why "force_assert(max_bytes == MAX_READ_LIMIT)" ? the max_bytes parameter is useless.

Code was modified to isolate openssl code, and this was an assumption in the code that, for the moment, is being preserved in order to reduce the code diff.  This is noted in the comment directly next to the code:
force_assert(max_bytes == MAX_READ_LIMIT); /*(code transform assumption; minimize diff)*/
Why bother asking this question?

connection_write_cq: why connection_read_cq inside connections-glue.c and connection_write_cq inside connections.c ?

Why ask this question?  connection_read_cq() was moved back to connections.c in 9481c102 after function pointers were introduced for these connection methods.

What's the meaning of "-glue" ?

Good question.  This is due to physical dependencies of the code and their callers.  connections.c is built into the main server.  connections-glue.c contains routines which might be called from modules.  connections-glue.c is built into a shared library on platforms such as Mac OSX where shared libraries (e.g. lighttpd modules such as mod_openssl) can not call back into symbols in the main executable.

array_insert_unique: free the buffer of tlsext_server_name without check the value. tlsext_server_name: free here but not allocated into the mod.

These are not questions.  The code (was) correct.  I think you may have been confusing con->tlsext_server_name with mod_openssl hctx->tlsext_server_name (specific to mod_openssl).  Besides con->tlsext_server_name and mod_openssl hctx->tlsext_server_name were both removed in a subsequent commit: 5f7d16a0.

config_insert: link ssl support to OpenSSL, but ssl may be supported by others libraries.

That is an attempt to preserve an existing user-friendly error, and no other TLS implementation modules currently exists (yet) for lighttpd.  The error might be extended or simply removed in future versions since other ssl.* directives will be reported as unrecognized directives (though only as warnings).

config_insert: unused-slot-moved-to-mod-openssl is a good idea, but for an embeded server, it's taken memroy for nothing.

This is really insignificant.  You're over-optimizing.  Besides, as with any optimization, please quantify the impact if you want the (perceived) issue to be taken seriously.  (Yes, this could be cleaned up in a separate commit, but would have made for a messier diff if it had been combined with the functional changes in the commit that substituted "unused-slot-moved-to-mod-openssl".)

mod_openssl_handle_con_accept: retrieves the specific_config of srv_socket with sidx, but the connection contains the specific_config. What is the difference ?

Good question.  The mod_openssl config context in the same config context as $SERVER["socket"] is retrieved with sidx, and this mod_openssl config context contains the initialized (SSL_CTX *) associated with (server_socket *), instead of storing (void *) data members in the (server_socket *) for the benefit of a specific module (mod_openssl).  The server has the values of some directives stored in (specific_config), and each module (potentially) has its own (plugin_config) with the values of the module's directives.

connection_init: use the macro's name "CLEAN" to initiliaze, is not a good idea. I think that a renaming should be interesting.

Out of scope.  While I don't like the CLEAN macro either, it matches current use in other modules.  You may choose to do this slightly differently in mod_mbedtls.  That's fine.

network_server_init: what is the link between openssl and the modification of the parameter of this function ? May another commit gives more explanation ?

See above explanation of the use of sidx to match the config context of (server_socket *) with the module config context.  This is not something previously needed by any other modules.

show_features: display SSL support for OpenSSL only.

As noted above, there are no other TLS modules currently available for lighttpd.  This command line output can be changed, but since it is merely informational, it is low priority.

connection_read_cq: I see that you are agree with my previous question. But do you known the meaning of "-glue" in this case ?

Why are you saying this?  I could not have "agreed with" your prior comment since both commits occurred well before you had the opportunity to look at any of these commits.  What is the purpose of your statement?    You could have just gone and deleted this and your prior comment, and retained the question asking about the difference between connection.c and connection-glue.c instead of asking the same question twice.  Please try to be concise when asking questions.  This should be a list of questions with appropriate context, not a storybook conversation.
As you can see, I am more than willing to answer your questions.  At the same time, I hope you take my criticism and try to ask more focused questions.  Cheers.

My code review allows to known the base code fastly. With it, I can send
questions.
I will read your answer, but first you can find here a very small patch for
your branch.
Thank you
2017-01-05 20:59 GMT+01:00 Glenn Strauss<EMAIL_ADDRESS>
@mchalain https://github.com/mchalain: I have tried to answer your
questions below, and I hope the answers are useful to you.
On the other hand, I question the utility of your "code review" comments
and remarks. Maybe you should omit your "code review" comments until you
are more familiar with the code base?
Overall, the patches aim to isolate the openssl code into a module,
separate from the main server, to allow an alternate module to be written
(e.g. mod_mbedtls) which provides similar functionality, also isolated from
the main server.
connection_read_cq: why "force_assert(max_bytes == MAX_READ_LIMIT)" ? the
max_bytes parameter is useless.
Code was modified to isolate openssl code, and this was an assumption in
the code that, for the moment, is being preserved in order to reduce the
code diff. This is noted in the comment directly next to the code:
force_assert(max_bytes == MAX_READ_LIMIT); /(code transform assumption;
minimize diff)/
Why bother asking this question?
connection_write_cq: why connection_read_cq inside connections-glue.c and
connection_write_cq inside connections.c ?
Why ask this question? connection_read_cq() was moved back to
connections.c in 9481c102 after function pointers were introduced for these
connection methods.
What's the meaning of "-glue" ?
Good question. This is due to physical dependencies of the code and their
callers. connections.c is built into the main server. connections-glue.c
contains routines which might be called from modules. connections-glue.c is
built into a shared library on platforms such as Mac OSX where shared
libraries (e.g. lighttpd modules such as mod_openssl) can not call back
into symbols in the main executable.
array_insert_unique: free the buffer of tlsext_server_name without check
the value. tlsext_server_name: free here but not allocated into the mod.
These are not questions. The code (was) correct. I think you may have been
confusing con->tlsext_server_name with mod_openssl hctx->tlsext_server_name
(specific to mod_openssl). Besides con->tlsext_server_name and mod_openssl
hctx->tlsext_server_name were both removed in a subsequent commit: 5f7d16a0.
config_insert: link ssl support to OpenSSL, but ssl may be supported by
others libraries.
That is an attempt to preserve an existing user-friendly error, and no
other TLS implementation modules currently exists (yet) for lighttpd. The
error might be extended or simply removed in future versions since other
ssl.* directives will be reported as unrecognized directives (though only
as warnings).
config_insert: unused-slot-moved-to-mod-openssl is a good idea, but for
an embeded server, it's taken memroy for nothing.
This is really insignificant. You're over-optimizing. Besides, as with any
optimization, please quantify the impact if you want the (perceived) issue
to be taken seriously. (Yes, this could be cleaned up in a separate commit,
but would have made for a messier diff if it had been combined with the
functional changes in the commit that substituted "unused-slot-moved-to-mod-
openssl".)
mod_openssl_handle_con_accept: retrieves the specific_config of srv_socket
with sidx, but the connection contains the specific_config. What is the
difference ?
Good question. The mod_openssl config context in the same config
context as $SERVER["socket"] is retrieved with sidx, and this mod_openssl
config context contains the initialized (SSL_CTX *) associated with
(server_socket *), instead of storing (void *) data members in the
(server_socket *) for the benefit of a specific module (mod_openssl). The
server has the values of some directives stored in (specific_config), and
each module (potentially) has its own (plugin_config) with the values of
the module's directives.
connection_init: use the macro's name "CLEAN" to initiliaze, is not a good
idea. I think that a renaming should be interesting.
Out of scope. While I don't like the CLEAN macro either, it matches
current use in other modules. You may choose to do this slightly
differently in mod_mbedtls. That's fine.
network_server_init: what is the link between openssl and the modification
of the parameter of this function ? May another commit gives more
explanation ?
See above explanation of the use of sidx to match the config context of
(server_socket *) with the module config context. This is not something
previously needed by any other modules.
show_features: display SSL support for OpenSSL only.
As noted above, there are no other TLS modules currently available for
lighttpd. This command line output can be changed, but since it is merely
informational, it is low priority.
connection_read_cq: I see that you are agree with my previous question.
But do you known the meaning of "-glue" in this case ?
Why are you saying this? I could not have "agreed with" your prior comment
since both commits occurred well before you had the opportunity to look at
any of these commits. What is the purpose of your statement?  You could have just gone and deleted this and your
prior comment, and retained the question asking about the difference
between connection.c and connection-glue.c instead of asking the same
question twice. Please try to be concise when asking questions. This should
be a list of questions with appropriate context, not a storybook
conversation.
As you can see, I am more than willing to answer your questions. At the
same time, I hope you take my criticism and try to ask more focused
questions. Cheers.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lighttpd/lighttpd1.4/pull/76#issuecomment-270741958,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD9CpCVleBoDpoVwu8NJlHdOXifDT99Iks5rPUuhgaJpZM4LYPjC
.

Ok I understand your criticism, and I'm sorry to importune you.
My point of view is that each commit has to build and run. Then I try to
understand each commit. I work like that for a long time, it's not easy to
change.
I understand the problem with "glue" now. I didn't see the library and
never think to the necessity , then I didn't understand, sorry. But with
the good option of the linker, it is possible to call the application
functions from any library on any platform ( with gcc the option is
-rdynamic).
As I told you it wasn't a review board, but I tried to understand some part
of code and know your point of view about others. Then my question about
"CLEAN" wasn't out of scope. I asked your point of view about it. My point
of view is that to use the word "CLEAN" every where, obfuscates the code,
and a future commit may be change this word every where.
idem for the unused-slot-moved-to-mod-openssl. I agree it is not a lot of
memory, But it's useless code, and may be removed in a future commit. If
you , now.agree to remove it, I can do it, but before to send an useless
patch, I prefer to ask you.
Best regards
2017-01-06 9:27 GMT+01:00 Marc Chalain<EMAIL_ADDRESS>
My code review allows to known the base code fastly. With it, I can send
questions.
I will read your answer, but first you can find here a very small patch
for your branch.
Thank you
2017-01-05 20:59 GMT+01:00 Glenn Strauss<EMAIL_ADDRESS>
@mchalain https://github.com/mchalain: I have tried to answer your
questions below, and I hope the answers are useful to you.
On the other hand, I question the utility of your "code review" comments
and remarks. Maybe you should omit your "code review" comments until you
are more familiar with the code base?
Overall, the patches aim to isolate the openssl code into a module,
separate from the main server, to allow an alternate module to be written
(e.g. mod_mbedtls) which provides similar functionality, also isolated from
the main server.
connection_read_cq: why "force_assert(max_bytes == MAX_READ_LIMIT)" ? the
max_bytes parameter is useless.
Code was modified to isolate openssl code, and this was an assumption in
the code that, for the moment, is being preserved in order to reduce the
code diff. This is noted in the comment directly next to the code:
force_assert(max_bytes == MAX_READ_LIMIT); /(code transform assumption;
minimize diff)/
Why bother asking this question?
connection_write_cq: why connection_read_cq inside connections-glue.c and
connection_write_cq inside connections.c ?
Why ask this question? connection_read_cq() was moved back to
connections.c in 9481c102 after function pointers were introduced for these
connection methods.
What's the meaning of "-glue" ?
Good question. This is due to physical dependencies of the code and their
callers. connections.c is built into the main server. connections-glue.c
contains routines which might be called from modules. connections-glue.c is
built into a shared library on platforms such as Mac OSX where shared
libraries (e.g. lighttpd modules such as mod_openssl) can not call back
into symbols in the main executable.
array_insert_unique: free the buffer of tlsext_server_name without check
the value. tlsext_server_name: free here but not allocated into the mod.
These are not questions. The code (was) correct. I think you may have
been confusing con->tlsext_server_name with mod_openssl
hctx->tlsext_server_name (specific to mod_openssl). Besides
con->tlsext_server_name and mod_openssl hctx->tlsext_server_name were both
removed in a subsequent commit: 5f7d16a0.
config_insert: link ssl support to OpenSSL, but ssl may be supported by
others libraries.
That is an attempt to preserve an existing user-friendly error, and no
other TLS implementation modules currently exists (yet) for lighttpd. The
error might be extended or simply removed in future versions since other
ssl.* directives will be reported as unrecognized directives (though only
as warnings).
config_insert: unused-slot-moved-to-mod-openssl is a good idea, but for
an embeded server, it's taken memroy for nothing.
This is really insignificant. You're over-optimizing. Besides, as with
any optimization, please quantify the impact if you want the (perceived)
issue to be taken seriously. (Yes, this could be cleaned up in a separate
commit, but would have made for a messier diff if it had been combined with
the functional changes in the commit that substituted
"unused-slot-moved-to-mod-openssl".)
mod_openssl_handle_con_accept: retrieves the specific_config of
srv_socket with sidx, but the connection contains the specific_config. What
is the difference ?
Good question. The mod_openssl config context in the same config
context as $SERVER["socket"] is retrieved with sidx, and this mod_openssl
config context contains the initialized (SSL_CTX *) associated with
(server_socket *), instead of storing (void *) data members in the
(server_socket *) for the benefit of a specific module (mod_openssl). The
server has the values of some directives stored in (specific_config), and
each module (potentially) has its own (plugin_config) with the values of
the module's directives.
connection_init: use the macro's name "CLEAN" to initiliaze, is not a
good idea. I think that a renaming should be interesting.
Out of scope. While I don't like the CLEAN macro either, it matches
current use in other modules. You may choose to do this slightly
differently in mod_mbedtls. That's fine.
network_server_init: what is the link between openssl and the
modification of the parameter of this function ? May another commit gives
more explanation ?
See above explanation of the use of sidx to match the config context of
(server_socket *) with the module config context. This is not something
previously needed by any other modules.
show_features: display SSL support for OpenSSL only.
As noted above, there are no other TLS modules currently available for
lighttpd. This command line output can be changed, but since it is merely
informational, it is low priority.
connection_read_cq: I see that you are agree with my previous question.
But do you known the meaning of "-glue" in this case ?
Why are you saying this? I could not have "agreed with" your prior
comment since both commits occurred well before you had the opportunity to
look at any of these commits. What is the purpose of your statement?
 You could have just gone and deleted
this and your prior comment, and retained the question asking about the
difference between connection.c and connection-glue.c instead of asking the
same question twice. Please try to be concise when asking questions. This
should be a list of questions with appropriate context, not a storybook
conversation.
As you can see, I am more than willing to answer your questions. At the
same time, I hope you take my criticism and try to ask more focused
questions. Cheers.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lighttpd/lighttpd1.4/pull/76#issuecomment-270741958,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD9CpCVleBoDpoVwu8NJlHdOXifDT99Iks5rPUuhgaJpZM4LYPjC
.

Sorry for the mistakes
2017-01-06 16:50 GMT+01:00 Marc Chalain<EMAIL_ADDRESS>
Ok I understand your criticism, and I'm sorry to importune you.
My point of view is that each commit has to build and run. Then I try to
understand each commit. I work like that for a long time, it's not easy to
change.
I understand the problem with "glue" now. I didn't see the library and
never think to the necessity , then I didn't understand, sorry. But with
the good option of the linker, it is possible to call the application
functions from any library on any platform ( with gcc the option is
-rdynamic).
As I told you it wasn't a review board, but I tried to understand some
part of code and know your point of view about others. Then my question
about "CLEAN" wasn't out of scope. I asked your point of view about it. My
point of view is that to use the word "CLEAN" every where, obfuscates the
code, and a future commit may be change this word every where.
idem for the unused-slot-moved-to-mod-openssl. I agree it is not a lot of
memory, But it's useless code, and may be removed in a future commit. If
you , now.agree to remove it, I can do it, but before to send an useless
patch, I prefer to ask you.
Best regards
2017-01-06 9:27 GMT+01:00 Marc Chalain<EMAIL_ADDRESS>
My code review allows to known the base code fastly. With it, I can send
questions.
I will read your answer, but first you can find here a very small patch
for your branch.
Thank you
2017-01-05 20:59 GMT+01:00 Glenn Strauss<EMAIL_ADDRESS>
@mchalain https://github.com/mchalain: I have tried to answer your
questions below, and I hope the answers are useful to you.
On the other hand, I question the utility of your "code review" comments
and remarks. Maybe you should omit your "code review" comments until you
are more familiar with the code base?
Overall, the patches aim to isolate the openssl code into a module,
separate from the main server, to allow an alternate module to be written
(e.g. mod_mbedtls) which provides similar functionality, also isolated from
the main server.
connection_read_cq: why "force_assert(max_bytes == MAX_READ_LIMIT)" ?
the max_bytes parameter is useless.
Code was modified to isolate openssl code, and this was an assumption in
the code that, for the moment, is being preserved in order to reduce the
code diff. This is noted in the comment directly next to the code:
force_assert(max_bytes == MAX_READ_LIMIT); /(code transform assumption;
minimize diff)/
Why bother asking this question?
connection_write_cq: why connection_read_cq inside connections-glue.c
and connection_write_cq inside connections.c ?
Why ask this question? connection_read_cq() was moved back to
connections.c in 9481c102 after function pointers were introduced for these
connection methods.
What's the meaning of "-glue" ?
Good question. This is due to physical dependencies of the code and
their callers. connections.c is built into the main server.
connections-glue.c contains routines which might be called from modules.
connections-glue.c is built into a shared library on platforms such as Mac
OSX where shared libraries (e.g. lighttpd modules such as mod_openssl) can
not call back into symbols in the main executable.
array_insert_unique: free the buffer of tlsext_server_name without check
the value. tlsext_server_name: free here but not allocated into the mod.
These are not questions. The code (was) correct. I think you may have
been confusing con->tlsext_server_name with mod_openssl
hctx->tlsext_server_name (specific to mod_openssl). Besides
con->tlsext_server_name and mod_openssl hctx->tlsext_server_name were both
removed in a subsequent commit: 5f7d16a0.
config_insert: link ssl support to OpenSSL, but ssl may be supported by
others libraries.
That is an attempt to preserve an existing user-friendly error, and no
other TLS implementation modules currently exists (yet) for lighttpd. The
error might be extended or simply removed in future versions since other
ssl.* directives will be reported as unrecognized directives (though only
as warnings).
config_insert: unused-slot-moved-to-mod-openssl is a good idea, but for
an embeded server, it's taken memroy for nothing.
This is really insignificant. You're over-optimizing. Besides, as with
any optimization, please quantify the impact if you want the (perceived)
issue to be taken seriously. (Yes, this could be cleaned up in a separate
commit, but would have made for a messier diff if it had been combined with
the functional changes in the commit that substituted
"unused-slot-moved-to-mod-openssl".)
mod_openssl_handle_con_accept: retrieves the specific_config of
srv_socket with sidx, but the connection contains the specific_config. What
is the difference ?
Good question. The mod_openssl config context in the same config
context as $SERVER["socket"] is retrieved with sidx, and this mod_openssl
config context contains the initialized (SSL_CTX *) associated with
(server_socket *), instead of storing (void *) data members in the
(server_socket *) for the benefit of a specific module (mod_openssl). The
server has the values of some directives stored in (specific_config), and
each module (potentially) has its own (plugin_config) with the values of
the module's directives.
connection_init: use the macro's name "CLEAN" to initiliaze, is not a
good idea. I think that a renaming should be interesting.
Out of scope. While I don't like the CLEAN macro either, it matches
current use in other modules. You may choose to do this slightly
differently in mod_mbedtls. That's fine.
network_server_init: what is the link between openssl and the
modification of the parameter of this function ? May another commit gives
more explanation ?
See above explanation of the use of sidx to match the config context of
(server_socket *) with the module config context. This is not something
previously needed by any other modules.
show_features: display SSL support for OpenSSL only.
As noted above, there are no other TLS modules currently available for
lighttpd. This command line output can be changed, but since it is merely
informational, it is low priority.
connection_read_cq: I see that you are agree with my previous question.
But do you known the meaning of "-glue" in this case ?
Why are you saying this? I could not have "agreed with" your prior
comment since both commits occurred well before you had the opportunity to
look at any of these commits. What is the purpose of your statement?
 You could have just gone and deleted
this and your prior comment, and retained the question asking about the
difference between connection.c and connection-glue.c instead of asking the
same question twice. Please try to be concise when asking questions. This
should be a list of questions with appropriate context, not a storybook
conversation.
As you can see, I am more than willing to answer your questions. At the
same time, I hope you take my criticism and try to ask more focused
questions. Cheers.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lighttpd/lighttpd1.4/pull/76#issuecomment-270741958,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD9CpCVleBoDpoVwu8NJlHdOXifDT99Iks5rPUuhgaJpZM4LYPjC
.

My point of view is that each commit has to build and run.

That is not an assumption you shared up front, and is not a universal truth.

Then I try to understand each commit.

That's fine.  Still, code reviews of a multi-commit branch should omit earlier remarks that are obsoleted by later commits in the same multi-commit dev branch.
In general, I, too, prefer each commit to be buildable.  However, I sometimes make an exception for large changes where a couple incremental patches make the diffs easier to review.  Were I not working so quickly to integrate my changes into a new dev branch for you, I probably would have done code moves in one commit, resulting in a broken branch, and then the code modifications to have it build in a second commit, so that only code changes were visible in the second commit, and the large code move commit could be ignored during review.
Regarding the acceptance of patches:

If you would like to contribute to lighttpd, please do so with functional, utilitarian patches instead of mere code aesthetics.
If code formatting and spelling in an old and stable open source project bother you so greatly that you can not write functional code, then I suggest trying to isolate your contributions so that you do not have to touch such "icky" code.

With the changes that I have presented in my dev branch, you should have to touch very little, if any, code outside of (a copy of) mod_openssl.c in order to write mod_mbedtls.c.  Inside mod_mbedtls.c, you may follow (just about) any reasonable coding convention as long as it is self-consistent.
If you would like to submit a patch which properly adjusts all the array assignments in configfile.c in order to remove "unused-slot-moved-to-mod-openssl", I will (hesitantly) consider such a patch, but would prefer if we focused on mod_openssl and mod_mbedtls.  In general, I will occasionally accept such patches, but such cleanups make using the commit history more difficult and cumbersome, as one has to iterate over and over through 'git annotate' to step past each and every "cleanup" patch to find functional changes of importance.  Again, I would prefer to focus on mod_openssl and mod_mbedtls.
If you would like to work with me to "clean up" mod_openssl.c, I will accept patches for that, such as replacing the CLEAN() macro.  On the other hand, I am not currently interested in "cleanup" patches that touch hundreds or thousands of lines of code across the entire codebase, so please do not spend time doing that.
.
IMNSHO, one (of numerous) measures of a senior programmer is the ability to work with a codebase and make improvements to it without first having to rewrite or touch many, many lines of code across the entire codebase.

My point of view is that to use the word "CLEAN" every where, obfuscates the code, and a future commit may be change this word every where.

While this is a possibility, it is also unlikely.  I feel that there are more important places to spend time on the lighttpd code base.  Additionally, mechanical changes to the code base are better done with a script, across the entire code base, and done sparingly (once in a long while), so as to disrupt the commit history as few times as possible.

I will read your answer, but first you can find here a very small patch for your branch.

Sorry, I did not see any patch attached.  Where might I find it?  Thanks.

In an effort to keep this pull request a bit more focused, I am planning to close this pull request ("Personal/mchalain/cleanup openssl") some time later this week unless there are requirements in your patches that have not be addressed by the patch set in my dev branch.
Are there any specific parts of your patches submitted in this pull request which should be discussed further?  Thanks.

I will read your answer, but first you can find here a very small patch for your branch.

Sorry, I did not see any patch attached. Where might I find it? Thanks.

no response.

Are there any specific parts of your patches submitted in this pull request which should be discussed further? Thanks.

no response.
.
As I noted above:

With the changes that I have presented in my dev branch, you should have to touch very little, if any, code outside of (a copy of) mod_openssl.c in order to write mod_mbedtls.c. Inside mod_mbedtls.c, you may follow (just about) any reasonable coding convention as long as it is self-consistent.

I hope you'll consider focusing on mod_mbedtls.c based off of the dev branch I provided which isolates openssl code into mod_openssl.c
If you have development questions, please ask in the lighttpd forums or on IRC freenode #lighttpd.
The lighttpd development forum is at: https://redmine.lighttpd.net/projects/lighttpd/boards/3


add importer info needed for recipe

Description
Add the info needed for the recipe in the top of the importer.

@arya-hemanshu can you have another look now?


Pre-commit linting

Overview
As stated from #4, anyone who committed to this repository must adhere to coding standards that have been set by eslint. However, the current solution requires contributors to run the lint process by themselves OR using the integrated Visual Studio Code plugin, which feels impractical for most cases.
This issue can be fixed by:

Add lint command to package.json that executes eslint directly
Add a pre-commit hook that executes the lint command.

:rocket: Issue was released in v0.1.0 :rocket:

:rocket: Issue was released in v0.1.0 :rocket:


ZeroDivisionError: float division by zero

When i run the program, i get the following error
File "/home/pi/Desktop/raspi code/MPU6050.py", line 825, in DMP_get_roll_pitch_yaw     a_grav_vect.z*a_grav_vect.z)) ZeroDivisionError: float division by zero
i am kind of new to python programming so i can't really make head or tails of this error, can you please help me with this

Hey!
It most likely means that the expression
        roll = math.atan(a_grav_vect.y /
                         math.sqrt(a_grav_vect.x*a_grav_vect.x +
                              a_grav_vect.z*a_grav_vect.z))

have a zero result for
math.sqrt(a_grav_vect.x*a_grav_vect.x +
                              a_grav_vect.z*a_grav_vect.z)

Now my memory of quaternions is rather rusty but I thought that this could never be the case. I'd check if the values passed to the function makes sense - do the math on paper if need be. Not sure what the right approach is here as I've not looked at this code in a long time.

Oh i see.... i will definitely see to it. Thanks
P.S. I am using the GPS module with a raspi 3 B+, does that have anything to do with the above mentioned error ??


MC 1.20.5 does not start.

Describe the problem
When trying to run Minecraft 1.20.5 I get the following error:
java.lang.UnsupportedClassVersionError: net/minecraft/bundler/Main has been compiled by a more recent version of the Java Runtime (class file version 65.0), this version of the Java Runtime only recognizes class file versions up to 61.0
Adding VERSION: "1.20.4" to run the previous version works.
Container definition
services:
  mc:
    image: itzg/minecraft-server
    restart: unless-stopped
    tty: true
    stdin_open: true
    ports:
      - "25666:25565"
    environment:
      EULA: "TRUE"
      SERVER_NAME: "Helvete, Lars-Olof!"
      MOTD: "§l§dLars-Olof's\n§4Minecraft Server"
      ENABLE_COMMAND_BLOCK: "TRUE"
      WHITELIST: |
        XXX
      EXISTING_WHITELIST_FILE: "MERGE"
      OPS: |
        XXX
      EXISTING_OPS_FILE: "MERGE"
      ENABLE_ROLLING_LOGS: "TRUE"
      TX: "Europe/Stockholm"
      MEMORY: "2G"
      USE_AIKAR_FLAGS: "TRUE"
    volumes:
      - ./data:/data

Container logs
+ exec mc-server-runner --stop-duration 60s java -XX:+UseG1GC -XX:+ParallelRefProcEnabled -XX:MaxGCPauseMillis=200 -XX:+UnlockExperimentalVMOptions -XX:+DisableExplicitGC -XX:+AlwaysPreTouch -XX:G1NewSizePercent=30 -XX:G1MaxNewSizePercent=40 -XX:G1HeapRegionSize=8M -XX:G1ReservePercent=20 -XX:G1HeapWastePercent=5 -XX:G1MixedGCCountTarget=4 -XX:InitiatingHeapOccupancyPercent=15 -XX:G1MixedGCLiveThresholdPercent=90 -XX:G1RSetUpdatingPauseTimePercent=5 -XX:SurvivorRatio=32 -XX:+PerfDisableSharedMem -XX:MaxTenuringThreshold=1 -Dusing.aikars.flags=https://mcflags.emc.gs -Daikars.new.flags=true -Xmx2G -Xms2G -Dlog4j.configurationFile=/data/log4j2.xml -jar minecraft_server.1.20.5.jar
Error: LinkageError occurred while loading main class net.minecraft.bundler.Main
        java.lang.UnsupportedClassVersionError: net/minecraft/bundler/Main has been compiled by a more recent version of the Java Runtime (class file version 65.0), this version of the Java Runtime only recognizes class file versions up to 61.0
2024-04-27T09:34:46.130Z        WARN    mc-server-runner        Minecraft server failed. Inspect logs above for errors that indicate cause. DO NOT report this line as an error.        {"exitCode": 1}
2024-04-27T09:34:46.130Z        INFO    mc-server-runner        Done

It seems like Minecraft 1.20.5 is compiled with java 21. You can see that in your logs as [...] has been compiled by a more recent version of the Java Runtime (class file version 65.0). Since you probably reused an older container that still comes java 17 you need to update your container image to a newer version. This might be done by explicitly run docker compose pull to get the newest version of the latest tag.
I myself fixed the issue by pinning the version of the container to the latest release 2024.4.1 eg.
services:
  mc:
    image: itzg/minecraft-server:2024.4.1
...

When starting with this config, docker will be forced to pull the specific container which ships with java 21. This has the benefit of always knowing what version of the Minecraft docker container is used.

Of course! Having latest in the compose file uses the locally cached version.  Why didn't I think of that? Oops! 😅


Removed the reference of Xamarin Studio

Replaced Xamarin Studio with Visual Studio for Mac

@prashantvc : Thanks for your contribution! The author, @conceptdev, has been notified to review your proposed change.

#sign-off


Restructure Kestrel docs

The docs page for using Kestrel has grown over time and is now too long - 69 minutes. And more content is coming for .NET 5. It needs to be split up and thought be put into how content is organized.
https://docs.microsoft.com/en-us/aspnet/core/fundamentals/servers/kestrel
Mostly this work will be reorganizing existing content, but I think a new Overview page that introduces Kestrel and summarizes features and links to other pages would be needed.
I don't have a proposal for content layout yet.

@JamesNK  when you have a proposed outline paste it here and I'll  try to implement it.

Here is a TOC that @Tratcher @halter73 and I came up with.
Most of the content exists today. Existing content could all use more work, but lets focus on splitting up what we have first. Future improvements can come later.

Overview

What is Kestrel

Features

Why use Kestrel (new)
Deploying Kestrel app - (new - related to https://github.com/dotnet/AspNetCore.Docs/issues/18411)
Kestrel in ASP.NET Core apps (aka hello world)
Summarize other pages (new)

Endpoint Bindings (Connection Logging is removed)

Launchsettings.json and precedence (future)
Code vs appsettings.json config
URL prefixes moves here

Configure Server Options

Code vs appsettings.json config

Transport configuration

#ifdef Libuv docs or warn that obsolete in 5

Protocol support

Overview of supported HTTP protocols (new)
HTTP/1.1

Request draining

HTTP/2

Requirements

HTTP/3 (future) @jkotalik

Requirements (future) @jkotalik

Logging and diagnostics

Configuring Kestrel logging (future)
Move Connection logging here
Event counters (future

Host filtering
When to use Kestrel with a reverse proxy

Talk about why you might want it
Performance implications (future)

Troubleshoot (future)

Client disconnection (future)
Common exceptions (future)

// @davidfowl

Why use Kestrel (new)

It’s true that in-proc means you no longer need to use Kestrel when running in IIS, but there are plenty of other scenarios for using Kestrel directly.
A)	Performance
B)	Quicker feature updates (e.g. gRPC)
C)	More customizable
D)	Cross platform (it’s the only AspNetCore server on Linux)

Why use Kestrel (new)

It’s true that in-proc means you no longer need to use Kestrel when running in IIS, but there are plenty of other scenarios for using Kestrel directly.
 Additionally, Kestrel has security hardening that makes it safe to use as an edge server (i.e. it can serve apps directly without requiring a proxy)

cc @tdykstra  if you're interested.

Some of the Kestrel pages have been split out. However, there is lots in the proposed TOC that hasn't been done.


Disable chat features in api unit tests

Ignore whatever chat settings in your local .env file.

Seems like a nice, simple way to unblock the tests. Do we intend to mock/fake chat features for these tests eventually? I could see a case being made either way, since we have discord-specific testing.

IMO It's not necessary. This code is 2 levels removed from the ChatService API. discord_lib has its own unit tests, and chat/ has unit tests that fake the service.

Seems like a nice, simple way to unblock the tests. Do we intend to mock/fake chat features for these tests eventually? I could see a case being made either way, since we have discord-specific testing.

IMO It's not necessary. This code is 2 levels removed from the ChatService API. discord_lib has its own unit tests, and chat/ has unit tests that fake the service.


UISEES-42 provide distinct permission values

Translations were not synced with changes in package.json, leading to
the appearance of duplicate displays in the UI. In fact, only the
translation values were duplicated; the actual permissionName values
were distinct.
Refs UISEES-42

@ViktorSoroka07 , can we please get this merged before the release? These identically-named permissions have been confusing for our POs; they would certainly be confusing for end-users as well.

@Godlevskyi, @Dmytro-Melnyshyn, can we please get this merged before the release? These identically-named permissions have been confusing for our POs; they would certainly be confusing for end-users as well.

Hi @zburke , sure we can merge it. Just looks like there is a merge conflict. Will you be able to resolve the conflict and merge it or do you want me to do it?

@Godlevskyi, this PR is now up to date and ready to merge. If it falls out of sync again please let me know and I'll merge master in again.

@Godlevskyi, this PR is now up to date and ready to merge. If it falls out of sync again please let me know and I'll merge master in again.

Thanks @zburke
Merged


Use Python 3.10

https://github.com/aws/aws-lambda-base-images/issues/31#issuecomment-1448638312

Done


NWRFC Forecast Temperature

"Forcings" (precip/airtemp) grids from NWRFC

Updated sample file: https://www.nwrfc.noaa.gov/weather/netcdf/2022/20221212/QTF.2022121212.nc.gz .

Note: It looks like only current day is posted, so the link above may not be very durable for retrieving a sample file after today. Will have to fiddle the date numbers in the URL.

QTF is in F, migration needs fixed by @adamscarberry

Local testing results:

Data acquisition and processing enabled in develop & stable; complete; closing issue


mock: Ensure VkICD_mock_icd.json uses up to date API version

closes #617

CI Vulkan-Tools build queued with queue ID 58196.

CI Vulkan-Tools build # 1201 running.

CI Vulkan-Tools build # 1201 passed.

GHA passed on my fork


Create 33-Search-In-Rotated-Sorted-Array.cs

File(s) Modified: 33-Search-In-Rotated-Sorted-Array.cs
Language(s) Used: c#
Submission URL: _https://leetcode.com/submissions/detail/793530321/_

Thanks, @notauserx!


add troubleshooting-masternode guide and fix links

migrate guide "Troubleshooting masternode: PoSe score and PoSe ban" from wiki on github to website
add baseurl to all links in /guide/index.html
adapt existing links to the guide

After this is merged, the wiki entry on github needs to be removed: https://github.com/firoorg/firo/wiki/Troubleshooting-masternode%3A-PoSe-score-and-PoSe-ban

conflicts fixed


DBS throttling

@vkuznet @bbockelm @amaltaro @belforte @h4d4
Hi Everyone,
I tested two versions of throttling while working with Valentin. V1: ported from crab, V2:  time based approach. Valentin made a PR for both versions in [2].
I used a hey tool written by Valentin using Go to create the concurrent access to DBS server on my VM. I had 30 threads for DBS sever. I used DBS API datatiers with a sleep of 0 to 3 second in side the API to simulate the various calls.  Here is an example:
/afs/cern.ch/user/v/valya/public/hey_linux -n 1000 -c 200 -U ./url.txt
url.txt is a file that has the urls to call, in this case it is
https://dbs3-test2.cern.ch/dbs/dev/global/DBSReader/datatiers?data_tier_name=LHE
this command send 200 concurrent datatiers API calls  to DBS and the total calls to send is 1000.
After the call hey tool will give stats. Please see
v2-t1-l100-n1000-c200.txt for the output.
I tested different combination, please see [1] for the results. limit is the number of concurrent calls, trange is the time range in V2.
[1] https://docs.google.com/spreadsheets/d/1pS0EHMglx67a-ktowDBiLBm1YbFZbSG8130iNbLHkrc/edit?usp=sharing
[2] https://github.com/dmwm/WMCore/pull/9211#issuecomment-495738944

Please feel free to ask questions or make comments about the tests. Any suggestion on what the limit we should place on  DBS is welcomed!  The tests were made on my VM, it will change when we move to int or prod. I will make more tests on int next week, But we got ideas on how the limits/throttling affect the output.
BTW, this PR changed the 500 error code to 429 that is what it should be. I did not update my local copy while doing tests. So the 500 in the table is the place holder for 429 that is "too many requests".

Hi Yuyi,
I have a few observations/questions to make, based on the gdoc results:

Where does the client error distribution comes from? E.g. the one at line 10, is that
135 + 188 client errors out of the 568 500 responses?
what's the unit of trange? secs or min?
what happens when the server crashes? can you still ping or ssh to it? or you need to
reboot the node via openstack interface? I'm curious to know whether it's a limitation of
a very small private VM or something else nuking and breaking the machine.

I'm not sure what to conclude out of those results, besides saying that V2 denies much more
requests.
I see you requested a new DBS deployment to testbed. Which throttling version is it using?
I'm very concerned about limiting DBS requests concurrency to only 3 (limit=3), reason
being that we share the same service certificate among many WMAgent nodes (~10 nodes).
Another case like that are the CMSWEB services calling DBS, those will all be made with
the DMWM service certificate, even though their source might be from different services.
Ideally speaking, I think we should have a short list of services that deserve a higher
limit (like any CMS service used in production), of course we still want to protect DBS
from those; then a very small limit for anything else, like grid jobs or anyone else
creating a script to monitor/analyze DBS data somehow. I know this gives more code maintenance
and not the best performance, but it might be considered in the near future in order to
provide a smooth production activity.

Hello Yuyi,
I've a comment regarding environment in which tests were executed.
If those request against DBS are following fronted-backend workflow and metrics such as: 'Requests/sec',  'Response time' , 'Latency', etc, have a high impact for the analysis of this test.
Please keep in mind that a VM have the fronted and the backed in the same node, while for example
in cmsweb-testbed, fronteds and backends are in different machines. Therefore, if the same test would be run in the testbed is probably that value for those metrics could increase.
Best Regards, Lina.

Alan,
let me answer few questions in line:
On  0, Alan Malta Rodrigues<EMAIL_ADDRESS>wrote:

Hi Yuyi,
I have a few observations/questions to make, based on the gdoc results:

Where does the client error distribution comes from? E.g. the one at line 10, is that
135 + 188 client errors out of the 568 500 responses?

The stats comes from hey tool itself. It works by generating concurrent requests
and collect the results back. If request arrives as 200 OK it will be used for
break-down metrics, e.g. req/sec, while if response fail to come or there is an
error it will be counted. The counters are per error type. Therefore in the
table 135 requests timeouts, while 188 never happen because the connection were
not established (again because underlying server were not responding).

what's the unit of trange? secs or min?

it is in seconds

what happens when the server crashes? can you still ping or ssh to it? or you need to
reboot the node via openstack interface? I'm curious to know whether it's a limitation of
a very small private VM or something else nuking and breaking the machine.

Yuyi should tell us exactly what happen.

I'm not sure what to conclude out of those results, besides saying that V2 denies much more
requests.
I see you requested a new DBS deployment to testbed. Which throttling version is it using?
I'm very concerned about limiting DBS requests concurrency to only 3 (limit=3), reason
being that we share the same service certificate among many WMAgent nodes (~10 nodes).
Another case like that are the CMSWEB services calling DBS, those will all be made with
the DMWM service certificate, even though their source might be from different services.

I agree with that and for that we can introduce white list of users/DN. If
everybody agree I can add additional code for that (it should be trivial).

Ideally speaking, I think we should have a short list of services that deserve a higher
limit (like any CMS service used in production), of course we still want to protect DBS
from those; then a very small limit for anything else, like grid jobs or anyone else
creating a script to monitor/analyze DBS data somehow. I know this gives more code maintenance
and not the best performance, but it might be considered in the near future in order to
provide a smooth production activity.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/dmwm/DBS/issues/607#issuecomment-496133193

@amaltaro @h4d4 @vkuznet
Let me answer questions by by the order asked.
Regarding the table  at line 10:
this is for test  " hey -n 1000 -c 200 limit=3"  which run 200 concurrent tests and total number of tests aew 1000, the limit=3 and with version 1 of throttling code.  So we expected "hey" gave us the status of all the 1000 calls made.  This test listed in line 8 and column B:
[200]        30 responses
[500]        568 responses
[502]        79 responses
server dead, machines crashed

30 calls ended with code 200, 568 calls ended with 500 ...
So total calls was responded were 30+568+79=677, then at line 10 there showed 135+188=323 calls were timeout.  After all 323+677=1000 calls were call reported.
When the server and node crashed, my ssh to the node was logged out automatically  I had to wait for a long time for the node recover itself , then I could ssh back. Sometimes I just went to openstack to hard restart the node, soft restart would not work.  Hard restart would be a bit quickly then waiting for it self-recovery. That's why these tests took long time.
The current deployment on cmsweb-testbed was v1.
I agree with the whitelist too. I also propose that we have a back list too. So we will be in better control of the system. Valentin, please add the additional code.
I also agree with Lina on the performance issue. I will test on cmsweb-testbed today.
Yuyi

@amaltaro @h4d4 @vkuznet @belforte @bbockelm
Please see tests' results on cmsweb_tested.
The tests were for v1. The first columns (B,C) were identical test conditions and tested with API datasets and datatiers. The columns (D,E) were identical test conditions and tested with API datasets, datatiers, filelumis (with lfn and dataset)  and filesummaries.
As you can see that the performance of the testing on cmsweb-testbed is much better than my VM.  I no longer got any server dead or node crashed.  Until row 9 was 100% executed.  One thing I did not quite understand was that the mixed APIs performed better than the simple datasets and datatiers that supposed to run faster.

I added more tests showed in 9-FG, 10-FG and 10-HIJ.
9-FG and 10-FG were both running two "hey" commands at the same time. 10-HIJ ran three.

@yuyiguo - looking over the Google Docs, I see:

When the concurrency in the client (hey) is set to N and the concurrency limit in the server is set to M, for throttling V1, if N>M, errors start to appear.  This is desired and the point of the throttle, correct?

The exceptions are when N=20 and M=20 (where 8% of queries got an error) as well as when M=3 and N=5 or 10.

For throttling V2, I'm having a harder time unraveling the results.  The performance on the low end (M=3) looks better but, for example, when the limit was set to 20 (M=20), there were no errors when the client used 100 parallel threads (N=200).

Is this the correct reading of the spreadsheet?

@bbockelm
Brian,
Your observation of  regrading the concurrency in the client (hey) N and concurrency limit in the server M was correct with the initial test on my VM  as sheet1. However if we look at sheet2 that is done on cmsweb_testbed that has two nodes,  all calls succeed when M=3, N=5/10/20/50 with total 100 calls.  Then when the client concurrency(N=5/20/50) and the concurrency limit(M=3) were kept the same, but the total number of calls increased, 1% calls was blocked. Here I saw the time concept started to play a role.
Lina just deployed DBS with M=10, I am testing with it now.

Brian,
V1 approach limits users on heavy APIs, as you said if user A send 5 queries
which takes 5 min each it will be blocked, but if user B send 10k short lived
queries they will be ok. What about if user B send 10k-100k queries and occupies
all DBS queue and may block other users at that time. The V1 approach does not
care about that because the server queue will be full by single user and
until his queries are cleared up others will have no access to the server.
V2 approach limits users by number of calls they made in time range N seconds.
(I don't think we should stick with 1 sec per se though). But it allows to spot
that user B sent too many queries in given time interval and we want to reduce
his/her capacity in order to give other users access to DBS. Please note, that
I may be wrong in implementation but that was the idea.
I think it outlines two different approaches to user access patterns and
sustainability of server in such condition. What we should use is our choice.
But what we need is to find through benchmark tests which parameters to use
to address (probably) both situations since we see them in DBS, i.e.
we have users whose queries consume lots of time in DBS, and we have users
who spread their queries and hit DBS at once and occupy all server capacity.
Valentin
On  0, Brian P Bockelman<EMAIL_ADDRESS>wrote:

Hi Valentin,
I am still having trouble understanding the V2 approach.  As I mentioned in PR #9211, with the current logic and trange>1, it appears perfectly acceptable for a single client to block all threads for the entire server in perpetuity.
Why do we care about limiting the number of queries in a certain time range?  If the user figures out a way to express their complex logic with a number of short, lightweight DBS queries -- great!  The current logic somewhat encourages the user to have a small number of heavy queries that block the server.
Another way to look at it -- if a single query lasts for more than 1 second (if trange=1), then it is "free" in the V2 model (the counter is reset once a second) but very costly to the user in the V1 model.  If trange=1, then 15 queries that each last 5 minutes are acceptable in V2, assuming they are done more than 1 second apart.  In the V1 model, if limit = 5, the 5 minute queries should be halted after 5.
The operating theory behind V1 is that the most precious resource is a server thread and limits the number of server threads a single identity (DN) can occupy at once.  This reduces the ability of a single actor to consume all the resources.
Brian
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/dmwm/DBS/issues/607#issuecomment-496961904

@vkuznet
Hi Valentin,
There are two kinds of timeout in the hey tool. One is hey times out for waiting for the server send back the result after submission and the other is hey times out if hey cannot submit the request. Do I understand correctly? If so, could you let me know how long each timeout is?  Are the values adjustable on the command line?
Yuyi

I tested with the client concurrency limit=10 on cmsweb-testbed.  Please see sheet3.
The result show that the throttling was not triggered with 250 concurrent clients and total 4000 API calls.  There are 11 calls of these 4000 were timeout.  I am thinking to make the concurrency limit=20 for next Tuesday's production release.   We have 5 production nodes and each has 30 threads. 30threads X 20/threads = 600 concurrent calls/user/node. So 20 should be big enough for not hit the limit?
I checked that the top caller was Dynamo that made 111.1K calls/hour at 1:00 May 14. Not sure how the 111k calls distributed in the hour.  If it hit the throttling limit, it would crash DBS anyway.

What about if user B send 10k-100k queries and occupies all DBS queue and may block other users at that time.

What queue are they blocking?  If you're worried about the accept queue -- that's at the TCP level and before any of the throttling is applied, hence the users can block the queue with or without the throttling (especially if it's from grid jobs and they aren't noticing the failures!).
I am not worried at all about 100k queries if they are doing them from a single thread.  I'm very, very worried if they are doing 10k queries from 1,000 worker nodes!

The result show that the throttling was not triggered with 250 concurrent clients and total 4000 API calls.

This is a strange result.  Potentially the frontend cannot process quickly enough to load up the backend?

We have 5 production nodes and each has 30 threads. 30threads X 20/threads = 600 concurrent calls/user/node

No.  5 production nodes x 30 threads is 150 threads total.  That means up to 150 concurrent calls total.  Given each user can have at most 20 active calls per backend, that means one user can have up to 100 concurrent calls.  So, 2 very badly behaved users can block the cluster.
Anyhow, I think 20 is a fine setting, assume it is a "V1"-style throttle.  There is some room for us to decrease it as needed.  Probably best to be a bit more forgiving and only decrease it if experience shows it is necessary.  This would have protected the cluster from the issue with the migration agent, for example.

Yuyi, you're correct.
The first one, the timeout hey awaits when server sends back the results.
The second one, when hey can't establish connection since server is busy.
According to the hey code it is controlled by
  -t  Timeout for each request in seconds. Default is 20, use 0 for infinite.

I don't see another timeouts in a code.
Best,
Valentin.
On  0, Yuyi Guo<EMAIL_ADDRESS>wrote:

@vkuznet
Hi Valentin,
There are two kinds of timeout in the hey tool. One is hey times out for waiting for the server send back the result after submission and the other is hey times out if hey cannot submit the request. Do I understand correctly? If so, could you let me know how long each timeout is?  Are the values adjustable on the command line?
Yuyi
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/dmwm/DBS/issues/607#issuecomment-497345250

Hi Valentin,
  -t  Timeout for each request in seconds. Default is 20, use 0 for infinite.

Could you update the code with separated controlling on two timeouts? I'd like  to see hey wait for 300 seconds on getting results back from the server, but waiting for 300 seconds to send a request seem not a realistic test.
Thanks,
Yuyi

Currently we are using v1 style throttling. Thanks Brain for the explaining the threads counting.   Lina, I will update the deployment to set  the throttling limit to 20.
We have a powerful 5th node in DBS, but it is used the same as the others. Is it possible that we increase its threads from 30 to 40, then add it twice in FE round robin list of nodes? I am not sure how exactly the FE assign requests to BE, this is just an idea to get our resource  fully used.

I retested "250 concurrent clients and total 4000 API calls" against cmsweb-testbed that had a throttling limit=10. The result confirmed the previous test with even better result as in the line 16 of sheet3 . Attached is the output of the hey and it shows that every call got result back except one was timeout.
[Uploading v1-n4000-c250.txt…]( hey results from -n4000 -c 250)

We have a powerful 5th node in DBS, but it is used the same as the others. Is it possible that we increase its threads from 30 to 40, then add it twice in FE round robin list of nodes? I am not sure how exactly the FE assign requests to BE, this is just an idea to get our resource  fully used.

Yuyi, the change round-robin in FE is not that easy. It requires changing FE
cmshost.pem module, see
https://github.com/dmwm/deployment/blob/master/frontend/cmshosts.pm#L242-L249
and to assign more weights on particular backend will require significant
code changes, e.g. we need to introduce weights and then use them in a code.
Then changes will be required to entire deployment procedure which
depends on prod/pre-prod/dev instances, etc.
I don't think we need it desperately now and I rather wait until we'll migrate
to k8s where dynamic pod provisioning will be possible.

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/dmwm/DBS/issues/607#issuecomment-497734596

Yuyi,
since I didn't write the code in a first place (I only adapted to use X509
proxies) it will require careful studying the entire structure. I can do it
eventually, but I can't guarantee that it will be soon since I have more
high-priority tasks through the summer.
Valentin.
On  0, Yuyi Guo<EMAIL_ADDRESS>wrote:

Hi Valentin,
  -t  Timeout for each request in seconds. Default is 20, use 0 for infinite.

Could you update the code with separated controlling on two timeouts? I'd like  to see hey wait for 300 seconds on getting results back from the server, but waiting for 300 seconds to send a request seem not a realistic test.
Thanks,
Yuyi
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/dmwm/DBS/issues/607#issuecomment-497729731

Thanks Valentin for explaining the FE code. Yes, we can wait for k8s.
Yuyi
From: Valentin Kuznetsov<EMAIL_ADDRESS>Reply-To: dmwm/DBS<EMAIL_ADDRESS>Date: Friday, May 31, 2019 at 2:58 PM
To: dmwm/DBS<EMAIL_ADDRESS>Cc: Yuyi Guo<EMAIL_ADDRESS>Mention<EMAIL_ADDRESS>Subject: Re: [dmwm/DBS] DBS throttling (#607)

We have a powerful 5th node in DBS, but it is used the same as the others. Is it possible that we increase its threads from 30 to 40, then add it twice in FE round robin list of nodes? I am not sure how exactly the FE assign requests to BE, this is just an idea to get our resource fully used.

Yuyi, the change round-robin in FE is not that easy. It requires changing FE
cmshost.pem module, see
https://github.com/dmwm/deployment/blob/master/frontend/cmshosts.pm#L242-L249https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_dmwm_deployment_blob_master_frontend_cmshosts.pm-23L242-2DL249&d=DwQCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=8bursUuc0V63OwREQMBG2Q&m=GCFgP69x9HY_zJ4H7EKpr1Q5blbjPj8FiRFDSHl9Uxo&s=Tg_kfZLTTGUhLsVpkCgu7T5qHmpIp9UZfC5CPnXXB1c&e=
and to assign more weights on particular backend will require significant
code changes, e.g. we need to introduce weights and then use them in a code.
Then changes will be required to entire deployment procedure which
depends on prod/pre-prod/dev instances, etc.
I don't think we need it desperately now and I rather wait until we'll migrate
to k8s where dynamic pod provisioning will be possible.

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/dmwm/DBS/issues/607#issuecomment-497734596https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_dmwm_DBS_issues_607-23issuecomment-2D497734596&d=DwQCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=8bursUuc0V63OwREQMBG2Q&m=GCFgP69x9HY_zJ4H7EKpr1Q5blbjPj8FiRFDSHl9Uxo&s=8JlFGO0uEKWFdXlCwPBo3272c8ppa_EJEgI4GCY7Pok&e=

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_dmwm_DBS_issues_607-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DAANROTRZ72NKOL2ARMRQZKLPYF7MVA5CNFSM4HPTMEHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWWHJNY-23issuecomment-2D497841335&d=DwMCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=8bursUuc0V63OwREQMBG2Q&m=GCFgP69x9HY_zJ4H7EKpr1Q5blbjPj8FiRFDSHl9Uxo&s=b4ScIJB_WZNRV1Ij2Hizeci2x4_g1vE4qE0DPASCqxA&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AANROTTQX7PG2LAKLU6Y5P3PYF7MVANCNFSM4HPTMEHA&d=DwMCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=8bursUuc0V63OwREQMBG2Q&m=GCFgP69x9HY_zJ4H7EKpr1Q5blbjPj8FiRFDSHl9Uxo&s=MQ6os0iQVFAk15cnFA-u8pi4iqIw1wAYgQkvknL0idc&e=.

Keep in mind that Lina is still commissioning 3 other general purpose backends. Which will bring us to 4 GP backends with twice'ish the resources as the previous backends (which will be retired once the transition is over).

Great news that Lina is already preparing 3 other new powerful general purpose backends. We did not plan for retiring the current 4 general purpose backends used by DBS and others without DBS storm.  So these machines are still in their lifespan and warranty?  Lina already successfully put in the first new general purpose machine in the backends.  My question is do we have to retire the four old BE machines? If not, could we keep the other service on these BEs and just move DBS into the 3 new BEs?  Would moving DBS only cause any technique difficulty?

Those are not physical machines, but allocations from a quota of virtual machines that "stay" while underlaying hardware comes and goes as it ages. Whether DBS and other backends can live on same VM's or really need to be moved to separate VM instance is a question that should be answered w/o a specific hardware configuration in mind. Then Lina and Caio can provision the resources. Yet it seems that after Unified was tamed and SQLAlchemy version problem understood, we are in same good shape as last N years. So this may be an item for the k8s setup, I do not know if the "VM" concept is still useful there.


[AVM Module Issue]: Unable to set support for accelerated networking for image definitions in the compute/gallery res module

Check for previous/existing GitHub issues

[X] I have checked for previous/existing GitHub issues

Issue Type?
Bug
Module Name
avm/res/compute/gallery
(Optional) Module Name if not listed above
No response
(Optional) Module Version
0.2
Description
This module contains image child module allowing to create image definition resources inside the Compute Gallery.
One important property of image definitions is isAcceleratedNetworkSupported which is set to false as a default in the child module.
However, this property is not exposed in the parent gallery module - lines 119-148, so the default value cannot be overwritten to true, even if the images parameter (array of objects) contain the property and the value.
(Optional) Correlation Id
No response

That's strange, dear bot, I can see that @ReneHezser does exist on GitHub 😄 .

👋🏻 Indeed. And I got the notification. I'll look for the problem.

Thanks @ReneHezser. I believe there is a quick fix for it, so if external contributions were allowed, I could have proposed a change via a PR. But I'll let you investigate this first.

Thanks @ReneHezser. I believe there is a quick fix for it, so if external contributions were allowed, I could have proposed a change via a PR. But I'll let you investigate this first.

External contributions are more than welcome :) But in this case @ReneHezser already open the PR 🚀


SKCanvas.Clear handles alpha incorrectly

I just updated from version 1.59.2 to 1.60.0 and noticed a strange behavior with my OpenGL-accelerated surface. I want to clear the surface to 0, 0, 0, 0 (fully transparent), but it seems like the Clear operation now multiplies the color with the existing values rather than setting them, which effectively makes Clear() a no-op with my settings. It worked fine in 1.59.2, and still works fine with my CPU backend.

I know this has been a long time, but I still have not been able to repro this. What platform was this on? Are you using Xamarin.Forms?

That's odd. I'll test some more on my end, and see if I can get a clean repro.
I'm on Win 10, working on my own engine (i.e. drawing to texture with OpenGL).

No problem. The issues that you notice will help everyone else.


Implemented options for num_quantiles feature in NumericStatsMixin

Original issue here.

 Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.Christopher Lee seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.You have signed the CLA already but the status is still pending? Let us recheck it.

Changes:

Added tests in test_numerical_options.py for num_quantiles
removed return in calculations

Fixed num_quantiles not passing the tests in the IntColumns, FloatColumns, TextColumns, and NumericStatsMixin tests.
Just need to implement test_num_quantiles_option

@clee1152 you also need to add scipy>=1.4.1,<1.11.0 on L 13 in your requirements.txt file ... that will resolve the graph_profile testing errors

Finished writing tests for num_quantiles feature. Tests in test_num_quantiles_option.py.

Migrated to new PR here.


Volatile Display on setRowData

Related to issue on enterprise version here
When I call setRowData the currently selected row and filtered column are reset. I'm getting around this with the following code (below), but this should probably be an option in setRowData.
var selected_currently = $scope.gridOptions.api.getSelectedNodes()[0];
var previous_filter_model = $scope.gridOptions.api.getFilterModel();

$scope.gridOptions.api.setRowData(orders.get());

if (selected_currently) $scope.gridOptions.api.selectNode(selected_currently, true, true);
if (previous_filter_model) $scope.gridOptions.api.setFilterModel(previous_filter_model);

for filter, see property newRowsAction
for selection, your right, it looses selection, however that is by design, as the 'new rows' are probably different rows. the workaround you have is the intention.


Expose fluent-bit configurations

There may be needs to fine tune the fluent-bit configuration and it should be possible to do so (eg multiline processing)

@SumoLogic/open-source-collection-team What do you think is better solution for this issue?

Expose only inputs configuration (handling specific file from configMap)
expose full configuration (mounting configMap as directory)

As this is advanced feature I would go with the second option

at this moment it is possible to overwrite sidecar configuration, see: https://github.com/SumoLogic/tailing-sidecar/blob/441c87117408eecd2c8a4ca6d8b72c074cc6058a/helm/tailing-sidecar-operator/values.yaml#L52C1-L62C16


C++ Rewrite/port?

I really like what you guys are doing with DAWproject and the fact that you can have daw agnostic project files.
It would be nice if someone could port this codebase to C++. Most daws these days use C++ so it wouldn't make much sense to add a java based dependency to the daws. Also, it would make the work of daw maintainers easier to implement DAWproject compatibility.
Some of the developers at LMMS had a brief discussion on their discord related to project files and DAWproject popped up. One of the devs there had this concern of adding java to our project but they all agree on adding DAWproject support.

There is actually not much to port. This is plain XML compressed in a ZIP file (use your favorite ZIP library). You can also go with any simple XML de-/serializer (e.g. https://github.com/zeux/pugixml). If you need wrapper classes they can be generated from the XML schemas: Project.xsd and Metadata.xsd.

issue relates to #39

What moss said.
Also there is no need to use java, just think of it as the reference implementation.
Making a C++ port depends on the approach though: if you just use off-the shelf ZIP and XML structures and read/write to a generic XML tree, I don't think writing a shared library will help you much as the majority of the code is about how to integrate with your own data structures. For validating the XML structure you can use the XSD files.
The other approach is to create a concrete wrapper classes resembling the element/attribute model which would then mirror the java version (which was used to generate the schemas in the first place). That could make sense as a deliverable others could reuse as it would be a bit more rigid and less error-prone than dealing with the generic model directly.
I did some experimentation on a branch (swift) to generate swift code from the java reflection, which is somewhat half-done, but that approach could be used to generate C++ classes. If somebody is interested in picking that up I'm happy to provide some guidance and eventually make that part of the main repo.

I started some c++ glue stuff a while ago and now created a draft pull request: #66
(rather unfinished but just want to mention it here)


Add support for grouping chats by timestamp

Merged in https://github.com/sourcegraph/jetbrains/pull/438


"Waiting for you to pay these expenses" is briefly displayed for approver

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/37866
PROPOSAL: https://github.com/Expensify/App/issues/37866#issuecomment-1986604092
Tests

Same as QA

[x] Verify that no errors appear in the JS console

Offline tests

Cannot be reproduced offline as you need the employee to send request to the approver
QA Steps

Precondition:

The Collect workspace has admin, employee and a non-reimburser approver.
Workspace has direct reimbursement.

[Employee] Go to workspace chat.
{Employee] Create a manual request and submit it.
[Approver] Go to workspace chat with employee.
[Approver] Click on the expense preview to go to expense report.
[Approver] Click Approve.

[x] Verify that no errors appear in the JS console

PR Author Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for the expected offline behavior in the Offline steps section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
[x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

[x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
[x] I followed proper code patterns (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
[x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
[x] I verified that comments were added to code that is not self explanatory
[x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method

[x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:

[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] I verified that if a function's arguments changed that all usages have also been updated correctly
[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))

[x] If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
[x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[x] If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
[x] If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:

[x] I verified that all the inputs inside a form are aligned with each other.
[x] I added Design label and/or tagged @Expensify/design so the design team can review the changes.

[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

iOS: mWeb Safari
https://github.com/Expensify/App/assets/110545952/332983ec-6f10-448f-94c1-4f2f27be582f

MacOS: Desktop
https://github.com/Expensify/App/assets/110545952/7db095f3-b058-4c9a-8448-5ded0939c246

Android: Native
https://github.com/Expensify/App/assets/110545952/34a85be0-0349-4a8f-88c0-cc39dfd2e297

Android: mWeb Chrome
https://github.com/Expensify/App/assets/110545952/15a492a8-e58f-4d26-8146-e9eaccda882d

iOS: Native
https://github.com/Expensify/App/assets/110545952/44f8f266-0a03-45f5-a49c-836350e51003

MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/110545952/bf2e2d4b-7945-42d9-8656-824c9935156d

:hand: This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@Gonals looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

Tests are fine

🚀 Deployed to staging by https://github.com/Gonals in version: 1.4.56-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

Bug #37866 not fixed and repro in all the platforms.
https://platform.applause.com/services/links/v1/external/5a478e8ad2af44886d9bc7fa802bcaffdb08931725eb1b5b435aaf545c60ffd6
https://platform.applause.com/services/links/v1/external/ee5f215ee7fa5c5f39ccd6118ff35d983f81adb1ae357d501b2876103d2af081

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.56-8 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅


Jar file crash at start (MacOsX)

Her is terminal error log
Last login: Sat Mar  9 17:15:54 on ttys000
Aramoks-iMac:~ aramok$ java -jar /Users/aramok/Downloads/skin_composer_mac.jar
Exception in thread "main" java.lang.ExceptionInInitializerError
at org.lwjgl.glfw.GLFW.glfwCreateWindow(GLFW.java:1831)
at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.createGlfwWindow(Lwjgl3Application.java:450)
at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.createWindow(Lwjgl3Application.java:394)
at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.createWindow(Lwjgl3Application.java:380)
at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.(Lwjgl3Application.java:108)
at com.ray3k.skincomposer.desktop.DesktopLauncher.main(DesktopLauncher.java:53)
Caused by: java.lang.IllegalStateException: GLFW windows may only be created on the main thread and that thread must be the first thread in the process. Please run the JVM with -XstartOnFirstThread. For offscreen rendering, make sure another window toolkit (e.g. AWT or JavaFX) is initialized before GLFW.
at org.lwjgl.glfw.EventLoop$OffScreen.(EventLoop.java:39)
... 6 more
AL lib: (EE) alc_cleanup: 1 device not closed
Aramoks-iMac:~ aramok$

To start the JAR version of Skin Composer on Mac, you have to launch it with a certain argument as explained in the error you see there.
Make sure you've downloaded the skin_composer_mac.jar. Open Terminal, navigate to the folder where Skin Composer is located. Then use the following command:
java -jar -XstartOnFirstThread skin_composer_mac.jar
I'm working on making this simpler and more reliable with an app package, but this is the best way to do it for now. Thank you for the report.


[Swift]variable properties

let -> var

Hello! This would be a good option and should be simple to contribute. Are you interested in contributing it?


Use tornado version compatible with flower 0.9.1

To https://github.com/AppScale/appscale/issues/2226

Successful build: https://ocd.appscale.com:8080/job/Daily Build/2240/


Backend interpreter

added runtime interpreter support for Local and Replication protocols (see PlaintextBackend)
fixed elaborator and parser so that we can parse intermediate programs correctly

redundant with https://github.com/apl-cornell/viaduct/pull/58


windows download url is incorrect

Installation fails on windows because the download url is incorrect (.tar.xz instead of .zip):
Expected: https://ziglang.org/builds/zig-windows-x86_64-0.10.0-dev.1261+6f986298c.zip source
Actual: https://ziglang.org/builds/zig-windows-x86_64-0.10.0-dev.1030+90059a12e.tar.xz -> returns HTTP 403

Thanks for catching and reporting this, should be fixed by https://github.com/pluvial/node-zig/commit/1420aca29870eeb7045e5e2acedcc007cae59063
Do give it another go and let me know if it's failing anywhere else.

Hey, thanks for looking into this.
I've tried it again, but its failing when running tar on the zip file. Log output:
 npm install -g @ziglang/cli
npm ERR! code 1
npm ERR! path C:\ProgramData\nvm\v16.13.2\node_modules\@ziglang\cli
npm ERR! command failed
npm ERR! command C:\WINDOWS\system32\cmd.exe /d /s /c node postinstall.js
npm ERR! $ which zig
npm ERR! /mnt/c/Program Files/nodejs/zig
npm ERR! $ zig version
npm ERR! Did not detect zig in system, will be installed locally
npm ERR! $ rm -rf $'C:\\ProgramData\\nvm\\v16.13.2\\node_modules\\@ziglang\\cli\\bin'
npm ERR! $ mkdir -p $'C:\\ProgramData\\nvm\\v16.13.2\\node_modules\\@ziglang\\cli\\bin'
npm ERR! $ curl -fsSL $'https://ziglang.org/builds/zig-windows-x86_64-0.10.0-dev.1301+cb3b1dd6d.zip' | tar xJ -C $'C:\\ProgramData\\nvm\\v16.13.2\\node_modules\\@ziglang\\cli\\bin' --strip-components=1
npm ERR! xz: (stdin): File format not recognized
npm ERR! tar: Child died with signal 13
npm ERR! tar: Error is not recoverable: exiting now
npm ERR! curl: (23) Failed writing body (2328 != 9260)
npm ERR! ProcessOutput {
npm ERR!   stdout: '',
npm ERR!   stderr: 'xz: (stdin): File format not recognized\n' +
npm ERR!   'tar: Child died with signal 13\n' +
npm ERR!   'tar: Error is not recoverable: exiting now\n' +
npm ERR!   'curl: (23) Failed writing body (2328 != 9260)\n',
npm ERR!   signal: null,
npm ERR!   exitCode: 2 (Misuse of shell builtins)
npm ERR! }
...

I should also add that It works perfectly in WSL so there is a workaround in windows.

You're right, this needs a more thorough rework to work well on Windows... As you mentioned it should work well within WSL, that's where I originally developed it 😅 But I'm not sure what the idiomatic way to do this kind of thing in Windows, might make sense to do the fetching + unzipping within node.js itself, or otherwise replace the Unixy calls with the CMD equivalent.

Hey folks, I opened #2 which should fix this problem. I'll appreciate it if you took a look 🙌


Neo4j::Node.find_nodes is broken in v4.0.4

Raises an ArgumentError when calling with correct args.
[30] pry(main)> begin
[30] pry(main)*   Neo4j::Node.find_nodes(:user, u, session)
[30] pry(main)* rescue ArgumentError => e
[30] pry(main)*   puts e.backtrace
[30] pry(main)* end
/Users/jmnsf/.rvm/gems/ruby-2.1.4/gems/neo4j-core-4.0.4/lib/neo4j-server/cypher_session.rb:163:in `find_nodes'
/Users/jmnsf/.rvm/gems/ruby-2.1.4/gems/neo4j-core-4.0.4/lib/neo4j/node.rb:199:in `find_nodes'

In node.rb:199 there's a call to session.find_nodes(label, value) but find_nodes is defined in the session as:
def find_nodes(label_name, key, value)
  value = "'#{value}'" if value.is_a? String
  response = _query_or_fail <<-CYPHER
    MATCH (n:`#{label_name}`)
    WHERE n.#{key} = #{value}
    RETURN ID(n)
  CYPHER
  search_result_to_enumerable_first_column(response)
end

Yielding a pretty obvious argument mismatch.

Interesting, I'm not sure I completely understand.  I have a few questions:

Is your label user or User?  (I ask because first letter uppercase is the typical convention)
What type of objects are u and session?

I can see that it's just interpolating value into there, but is not good.  I think that this should be replaced by a Query chain which will escape/parameterize values for us.
For now, I'd suggest you'd use a query directly, like this:
Neo4j::Session.current.query.match(n: :Label).where(n: {key: value}).pluck(:n).first

I'm starting out with Neo4j, just testing a few things and importing data, so I wasn't aware of the conventions; thank you for pointing that out. The label is, indeed, user.
u would be a ruby hash and session is Neo4j::Session.current!.
As you said, the implementation doesn't make a lot of sense. Neo4j::Node.find_values receives a tag and something identified as value which one assumes could be a hash of { key => values } to match in the query, but then the implementation itself is broken.
Like I mentioned, It calls session.find_nodes(label, value) when the signature of the method expects 3 arguments: label, key and value.
To my understanding, the session's find_nodes method isn't correct either, it should accept a hash of values to match instead of a single key/value pair, and then interpolate those into a MATCH query.
This is just my general, never-used-a-graph-db, opinion though, perhaps this architecture does make sense for a Graph use case and I'm just simply not aware of it due to inexperience.

Yeah, I'm definitely not defending some of the methods here.  Since making the Query class and creating the neo4j gem with ActiveNode modeling, I'm thinking that a lot of the methods like this on Session / Node should go (@subvertallchris do you concur?)
So firstly, I think that find_nodes doesn't take a hash and a session like you're using it.  It looks like it takes a label and a single key and value.  So if you wanted to find a user by E-Mail, you might do:
Neo4j::Node.find_nodes(:user, :email<EMAIL_ADDRESS>
If you have a hash of keys/values, I would suggest using Query like this (assuming your Hash variable is called attributes:
Neo4j::Session.query.match(var: :user).where(attributes).return(:var) # Will give you an Array of results where you can call `var` on each to get the value

# If you want to get the objects directly:
Neo4j::Session.query.match(var: :user).where(attributes).pluck(:var) # Will give you an Array of `user` nodes

And, of course, I always recommend you check out ActiveNode in the neo4j gem, where you can do something like this:
class User
  include Neo4j::ActiveNode
  self.mapped_label_name = 'user' # If you used a capital `U` as your label then this line isn't needed
end

Then you could simply do User.where(attributes).to_a

@cheerfulstoic you basically wrote what I was typing up. :-) I'm not sure this method should go completely -- some basic shortcuts for querying may be worth keeping around -- but they definitely need some love.
@jmnsf, your hunch here is totally correct. I've noticed this method while refactoring and always meant to track it down, see how/where it's used, but never got around to it. We should rework it for the upcoming release.

@cheerfulstoic I wanted to avoid ActiveNode because I'm trying to draw some performance metrics for my specific use case, which means I need to be able to understand/change the Cypher queries being done to the DB, so I ended up on neo4j-core's middleground. I will probably use eventually if I end up using Neo4j.
@subvertallchris I guess the reason I stumbled on this is because it's one of the most stand out methods in the Node docs and represents something you'd commonly want to do with nodes. When you're starting out and unaware of conventions you kinda just stumble around the docs trying to find ways to do what you want.
Thanks for the help on this guys, did end up doing session.query and session.find_nodes, working fine through that!

This is apparently still a problem in neo4j-core v5.1.11  :P

I get a similar problem with the args mismatch in 6.1.1 for find_nodes --

This is perhaps not a satisfying answer, but find_nodes is deprecated as of version 7.0 of neo4j-core (final version just released yesterday, see this blog post).
Basically there is an entirely new API in neo4j-core which is based around simply making Cypher queries and getting back Node / Relationship / Path objects.  The old API is still around, but neo4j-core was doing a lot of different things and we wanted to simplify, especially since you can use ActiveNode and ActiveRel for higher level abstractions in the neo4j gem.
This doesn't mean we wouldn't welcome another level in the Neo4j.rb world which is above neo4j-core and below neo4j.
Questions / discussion welcome here or on our Gitter channel


strip unnecessary quotes for notify-url configurations

This PR will conveniently strip surrounding quotes (either " or ') from --notify-url or --slack-hook-url argument values to ensure that there are no unexpected parsing errors downstream.
I don't think this will affect folks who use the helm chart, but because this is finitely scoped it's probably worth doing this automatically as a convenience to protect users.

cc @johnwc


would it work with angular2

Just want to check if there is similar project for ion range slider which supports angular2

+1
Thanks,