improve the quick start to handle ES index publish

Currently our quick doesn't handle pushing / creating ES index for search discovery. We need to add into our sample script.

cc @jinhyukchang

Hi guys, I would prepar the fix for this, but I first need to understand one thing.
If I run the services from docker-compose, the config files are copied into image during docker build process.
But as I understand it, the Elasticsearch index has to be unique, an example in the code below line
'publisher.elasticsearch.{}'.format(ElasticsearchPublisher.ELASTICSEARCH_NEW_INDEX_CONFIG_KEY): elasticsearch_new_index_key+str(rand)

--
def create_sample_job(table_name, model_name):

    from elasticsearch import Elasticsearch
    es = Elasticsearch([
        {'host': '<IP_ADDRESS>'},
    ])

    tmp_folder = '/var/tmp/amundsen/{table_name}/search_data.json'.format(table_name=table_name)
    node_files_folder = '{tmp_folder}/nodes'.format(tmp_folder=tmp_folder)
    relationship_files_folder = '{tmp_folder}/relationships'.format(tmp_folder=tmp_folder)

    task = DefaultTask(loader=FSElasticsearchJSONLoader(),
                       extractor=Neo4jSearchDataExtractor(),
                       transformer=ElasticsearchDocumentTransformer())

    elasticsearch_client = es
    elasticsearch_new_index_key = 'tables'
    elasticsearch_new_index_key_type = 'table'
    elasticsearch_index_alias = 'ia'

    rand = str(random.randint(0,1000))
    print(rand)

    job_config = ConfigFactory.from_dict({

        'extractor.search_data.extractor.neo4j.{}'.format(Neo4jExtractor.GRAPH_URL_CONFIG_KEY): neo4j_endpoint,
        'extractor.search_data.extractor.neo4j.{}'.format(Neo4jExtractor.MODEL_CLASS_CONFIG_KEY): 'databuilder.models.neo4j_data.Neo4jDataResult',
        'extractor.search_data.extractor.neo4j.{}'.format(Neo4jExtractor.NEO4J_AUTH_USER): neo4j_user,
        'extractor.search_data.extractor.neo4j.{}'.format(Neo4jExtractor.NEO4J_AUTH_PW): neo4j_password,
        'loader.filesystem.elasticsearch.{}'.format(FSElasticsearchJSONLoader.FILE_PATH_CONFIG_KEY): tmp_folder,
        'loader.filesystem.elasticsearch.{}'.format(FSElasticsearchJSONLoader.FILE_MODE_CONFIG_KEY): 'w',
        'transformer.elasticsearch.{}'.format(ElasticsearchDocumentTransformer.ELASTICSEARCH_INDEX_CONFIG_KEY): elasticsearch_new_index_key+str(rand),
        'transformer.elasticsearch.{}'.format(ElasticsearchDocumentTransformer.ELASTICSEARCH_DOC_CONFIG_KEY): elasticsearch_new_index_key_type,
        'publisher.elasticsearch.{}'.format(ElasticsearchPublisher.FILE_PATH_CONFIG_KEY): tmp_folder,
        'publisher.elasticsearch.{}'.format(ElasticsearchPublisher.FILE_MODE_CONFIG_KEY): 'r',
        'publisher.elasticsearch.{}'.format(ElasticsearchPublisher.ELASTICSEARCH_CLIENT_CONFIG_KEY): elasticsearch_client,
        'publisher.elasticsearch.{}'.format(ElasticsearchPublisher.ELASTICSEARCH_NEW_INDEX_CONFIG_KEY): elasticsearch_new_index_key+str(rand),
        'publisher.elasticsearch.{}'.format(ElasticsearchPublisher.ELASTICSEARCH_ALIAS_CONFIG_KEY): elasticsearch_index_alias
    })
    job = DefaultJob(conf=job_config,
                     task=task,
                     publisher=ElasticsearchPublisher())
    return job

Because otherwise publisher tries to create the same index and it fails. But if I have config files built in docker images, then I have to change the config in here https://github.com/lyft/amundsensearchlibrary/blob/406fbb01a8f3c35ce7e8aa8583b1215c0901ead8/search_service/config.py#L23 and rebuild the image or write the ElasticSearch index in config with wildcards like tables_* or change config and provide it to the container via environment variable during run, but then I have to restart the container with searchservice.
Do you have any idea, how you want to do it? There are definitely few more ways how to do it, but I am asking if you have any preference and of course if I understand it properly :)
Thanks!

Hey @jhettler ,
I think your suggestion of using random should be good. In fact, we are using date and uuid into the suffix of index to decrease probability of collision in our production and it has been run fine for more than 6 months.
I know that old index will be deleted once swapped, but I would suggest to go with uuid to minimize collision case.
Example:
elasticsearch_new_index = 'table_search_index_{ds}_{hex_str}'.format(ds=kwargs['ds'],                                                                          hex_str=macros.uuid.uuid4().hex)

@jhettler ,
As Jin mentioned, we(Lyft) internally deleted the the index once swap. So either with UUID or random id should be fine. Apart from that, I think you get all the config correct for ES publisher job by comparing with our internal Airflow task.
One thing worth mentioned is that you may want to update https://github.com/lyft/amundsenfrontendlibrary/blob/master/docker-amundsen.yml to start the ElasticSearch container as well in the docker-compose. Let us know if you get it working or need us any help. Thanks a lot for the contribution :)

That's all clear what you wrote above. I have all working at the moment. The question is how to update configuration for search service. ... because if you create a new index in Elasticsearch (and swap it with the old one), you have to change it in configuration here https://github.com/lyft/amundsensearchlibrary/blob/406fbb01a8f3c35ce7e8aa8583b1215c0901ead8/search_service/config.py#L23 otherwise search service won't work anymore because it is configured against old index.
How do you update the configuration of the search service after creating new index in Elasticsearch? Because if it's in docker I have to at least restart the search service container with the new config (changed ELASTICSEARCH_INDEX = 'table_search_index') if the config is provided via environment variable into docker container or worse rebuild whole docker image if I change it in the code mention in link above in this comment.
Thanks for the explanation!

Could a way to resolve this simply be to refer to the alias on the consuming search service side?
I'm not super familiar with ES but https://www.elastic.co/guide/en/elasticsearch/guide/current/index-aliases.html#index-aliases states:

An index alias is like a shortcut or symbolic link, which can point to one or more indices, and can be used in any API that expects an index name. Aliases give us an enormous amount of flexibility. They allow us to do the following: ....

To me the above together with https://github.com/lyft/amundsendatabuilder/blob/master/databuilder/publisher/elasticsearch_publisher.py#L164-L174 already in the code suggests that the alias is just as good to use on the search side (yeah, naming things is one of the harder parts of software 😉)
Oh, and also: Sorry for trying to mingle here, but for me getting search working to be able to do a more convincing internal demo here is next up with Amundsen. So I'm quite keen on seeing this resolved as well 😄

Hey Jakub,
I am away from keyboard so I will read through your comment later on but
our ES publisher already uses alias and does the index swap and deletes old
index. All we need to set is new random index name and alias that you will
be using and let the search service point to alias — not the index name.
Hope this helps.
-Jin
On Wed, Apr 17, 2019 at 4:58 AM jornh<EMAIL_ADDRESS>wrote:

Could a way to resolve this simply be to refer to the alias on the
consuming search service side?
I'm not super familiar with ES but
https://www.elastic.co/guide/en/elasticsearch/guide/current/index-aliases.html#index-aliases
states:
An index alias is like a shortcut or symbolic link, which can point to
one or more indices, and can be used in any API that expects an index name.
Aliases give us an enormous amount of flexibility. They allow us to do the
following: ....
To me the above together with
https://github.com/lyft/amundsendatabuilder/blob/master/databuilder/publisher/elasticsearch_publisher.py#L164-L174
already in the code suggests that the alias is just as good to use on the
search side (yeah, naming things is one of the harder parts of software
😉)
Oh, and also: Sorry for trying to mingle here, but for me getting search
working to be able to do a more convincing internal demo here is next up
with Amundsen. So I'm quite keen on seeing this resolved as well 😄
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lyft/amundsendatabuilder/issues/21#issuecomment-484052865,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AOSuBV9D3UZILoQGfa9gPXyQcVxwoZuvks5vhwxjgaJpZM4crDUM
.

+1 @jinhyukchang , the related code could be found in https://github.com/lyft/amundsendatabuilder/blob/master/databuilder/publisher/elasticsearch_publisher.py

I am in a conference today, will take a look at the comment in the night.

That's all clear what you wrote above. I have all working at the moment. The question is how to update configuration for search service. ... because if you create a new index in Elasticsearch (and swap it with the old one), you have to change it in configuration here https://github.com/lyft/amundsensearchlibrary/blob/406fbb01a8f3c35ce7e8aa8583b1215c0901ead8/search_service/config.py#L23 otherwise search service won't work anymore because it is configured against old index.
How do you update the configuration of the search service after creating new index in Elasticsearch? Because if it's in docker I have to at least restart the search service container with the new config (changed ELASTICSEARCH_INDEX = 'table_search_index') if the config is provided via environment variable into docker container or worse rebuild whole docker image if I change it in the code mention in link above in this comment.
Or you have to set something like ELASTICSEARCH_INDEX = 'table_search_index*' in search config - use the wildcards in Elastic search index.
Thanks for the explanation!

@jhettler In search service configuation, you can simply add the alias name into the index name, and Elasticsearch figures out which index it needs to read from.
ELASTICSEARCH_INDEX = <alias name>

Could a way to resolve this simply be to refer to the alias on the consuming search service side?
I'm not super familiar with ES but https://www.elastic.co/guide/en/elasticsearch/guide/current/index-aliases.html#index-aliases states:

An index alias is like a shortcut or symbolic link, which can point to one or more indices, and can be used in any API that expects an index name. Aliases give us an enormous amount of flexibility. They allow us to do the following: ....

To me the above together with https://github.com/lyft/amundsendatabuilder/blob/master/databuilder/publisher/elasticsearch_publisher.py#L164-L174 already in the code suggests that the alias is just as good to use on the search side (yeah, naming things is one of the harder parts of software 😉)
Oh, and also: Sorry for trying to mingle here, but for me getting search working to be able to do a more convincing internal demo here is next up with Amundsen. So I'm quite keen on seeing this resolved as well 😄

+1
This was a miss on our side and will fix it shortly, but happy to see solved by open source community as well as @jhettler proposed :)

I should have it done today EOD :)

with @jhettler 's https://github.com/lyft/amundsendatabuilder/pull/23, this issue could be closed now :)


cpprest dependency needs to be updated for NuGet package (fails to compile in latest VS)

The latest VS 2017 compiler fails to build cppreset v2.9. The issue were fixed as of cpprest v2.10.1, but the library owners are no longer publishing to NuGet (suggesting use of vcpkg instead).
https://github.com/Microsoft/cpprestsdk/issues/655
https://github.com/Microsoft/cpprestsdk/commit/31b0e47b3a94c893b5cddd48af742cc911cd1cad
https://github.com/Microsoft/cpprestsdk/commit/05e474a2d8f041f56691a4e9f7f79bd0f110809c

Thanks for the intel.  We'll keep an eye out

Which VS 2017 compiler are you looking at that it fails for?  I'm using 15.6.7 and it seems to build fine.

Odd, it builds fine for us on that version of VS2017.
How do I repro this issue?  Are you just building https://github.com/Microsoft/xbox-live-api/tree/master/Build/Microsoft.Xbox.Services.140.UWP.Cpp by itself with no edits and it errors for you?
I thought VS wouldn't break existing code

Should be fixed now


Issue template for feedback

Create an issue template for participants to provide feedback during/after the workshop

I wouldn't use issues for inviting feedback in another workshop. But here, it probably makes sense to exemplify some possibilities on GitHub like issue template, check lists in markdown etc.

If you don't think it makes sense (why not?), we don't have to. We can show the issue template functionality in the actual project repo, where participants will open issues for bugs they find.

Closing. We may adjust the template later.


Some additional processing

I am opening this issue following the discussion in #460 in order to not mix different topics, which I guess is easier to handle for you.

@melsophos what kind of math strangeness?

Typically this: \textdollar{}{{S}}\^{}1 \ensuremath{\backslash}times {{S}}\^{}3\textdollar{}, instead of the simple $S^1 \times S^3$.

And you can avoid the brackets by either sentence-casing your titles (which is what zotero recommends) or setting the language to non-English.

I prefer to keep the same casing as the one in the title of the paper, and in particular to keep accents for english titles. Moreover this is necessary when there are abbreviation. What I wanted to say is that the complicated-looking title
{{{BV Master Action}} for {{Heterotic}} and {{Type II String Field Theories}}}

can be replaced by the simpler one
{{BV Master Action for Heterotic and Type II String Field Theories}}

But after reading several sites on the topic, I am not sure that this is the good solution: it seems that one should protect only things like first name and abbreviation, and let the bibtex style capitalize or not. Note that I don't like so much this solution either because if I have both french and english titles in my bibliography I don't know if bibtex will use different rules.

Yes! Thank you, separate issue much appreciated. In order
math mode
I assume you have $S^1 \times S^3$ somewhere in your title, and you want that output in math mode. BBT can't know whether you wanted math mode or whether you wanted literally $S^1 \times S^3$, in which case \textdollar{}{{S}}\^{}1 \ensuremath{\backslash}times {{S}}\^{}3\textdollar{} is actually correct. If you want untranslated LaTeX output, you can have titles that have
$S^1 \times S^3$
 in them and BBT will pass on the part between
...
 unmodified.
sentence casing
Indeed, style capitalization is why BBT doesn't surround the whole text in double-braces. The double-double braces are another workaround to prevent things like https://github.com/retorquere/zotero-better-bibtex/issues/383#issuecomment-153904326 . If you sentence-case your Zotero references, BBT will Title Case them by default or for references marked as English, and will keep them unchanged (whatever sentence or titlecasing you might have otherwise) if not. Which is odd, but apparently specced behavior (see same thread)

Wow that last sentence makes no sense. Here's what I meant:
sentence casing
Indeed, style capitalization is why BBT doesn't surround the whole text in double-braces. The double-double braces are another workaround to prevent things like https://github.com/retorquere/zotero-better-bibtex/issues/383#issuecomment-153904326 . The idea is that you sentence-case your Zotero references (as Zotero recommends), and then it gets a little complicated.
There are two things that affect capital letters: Title Casing and Capital Preservation.
Capital Preservation is always on for certain fields like titles. If you have capitals in a word except the first, BBT assumes you meant it to be there, and you want BibTeX to leave it alone no matter what. To do that, it wraps those (strings of) words in double braces. This is to let BibTeX know that ISDN may not be changed to isdn or Isdn.
For English titles (and if no language is set, it is assumed to be -- I know, how very un-post-colonial of me), it will Title Case the title on output, but those Title Cased words BBT changed itself will not be wrapped in double-braces, as it is OK for the styles to change casing for those, depending on the style at play. So I like ISDN heaps better than dialup would output to I Like {{ISDN}} Heaps Better than Dialup. Apparently non-English titles are supposed to be in sentence case, so BBT doesn't touch those. See again this thread: https://github.com/retorquere/zotero-better-bibtex/issues/383#issuecomment-153904326 . Bib(La)TeX be crazy.

For the math issue, see also the Hardcore page on the wiki.

That would be the "From unicode" page, sorry

Correctly, I have $S^1 \times S^3$ in the title and would like to keep it like this, such that Latex displays it nicely as a formula. But I understand that it might get tricky to recognize precisely what is a formula and what is not. On the other hand I don't like to add
 because this adds more text inside Zotero fields and display, but I prefer to keep it clean.
For the title I understand better the point, I never thought thoroughly to the problem. I think I should remove from my script this double bracing and keep the default behaviour, the problem is that I don't want to lowercase all the titles of my entries, there are too many of them! (since the capitalization is preserved when saving the document). I will think to this but I am not sure that you should change anything in BBT.

It's not just that it's tricky -- the intent is to have LaTeX render what the user typed, and if the user typed math formulas in LaTeX look like $S^1 \times S^3$ to math formulas in LaTeX look like $S^1 \times S^3$ it would render to "math formulas in LaTeX look like S1 x S3", which is not what the user entered. If you want to export LaTeX as LaTeX without
, add the tag #LaTeX to the reference, and it will assume you want LaTeX to be exported unchanged for most of its fields (things like URLs etc are always escaped). But there must be a way to signal "I want dollar signs" or "I want math mode". It's not safely detectable.
You don't have to lowercase the titles; if you want them Title Cased, the bracing will preserve that. Zotero has built-in functions to changes titles to sentence/title case BTW.

Anything else I can explain on this topic? Anything you disagree with? If I missed something I'd love to improve BBT.

Thanks a lot for all these explanations; and sorry for the late answer, these last days were busy. The #LaTeX tag is a good idea!


[🐛 BUG] Tracking does not work if device_tracker is bound to person

System Health details

Version
core-2022.9.5

Installation Type
Home Assistant Core

Development
false

Supervisor
false

Docker
false

User
homeassistant

Virtual Environment
true

Python Version
3.10.6

Operating System Family
Linux

Operating System Version
5.4.70-ravor

CPU Architecture
x86_64

Timezone
Europe/Berlin

Configuration Directory
/home/homeassistant/.homeassistant

Checklist

[X] I have enabled debug logging for my installation.
[X] This issue only contains 1 issue (if you have multiple issues, open one issue for each issue).

Describe the issue
Hi,
in my HA instance all trackable users are configured as persons (as in "settings" -> "people"). For some reason, the person is always displayed as "unknown" or "away" instead of the location. Even in a map card, the person is not displayed. If I choose to use the device tracker entity directly, it is displayed as it should.
Could you check that?
Reproduction steps

Create a person in "settings" -> "people"
Associate the device_tracker entity from the watch to the person
Add person to UI or to map card

Expected outcome:
Person should be displayed like device_tracker entity
Actual outcome:
Person does not have a location
Debug logs
Data is fetched:
Sep 19 12:57:30 hwr-server01 hass[11602]: 2022-09-19 12:57:30.380 DEBUG (MainThread) [custom_components.xplora_watch.coordinator] Fetch data from Xplora®: 6983592
Sep 19 12:57:30 hwr-server01 hass[11602]: 2022-09-19 12:57:30.686 DEBUG (MainThread) [custom_components.xplora_watch.coordinator] load address from openstreetmap.org
Sep 19 12:57:30 hwr-server01 hass[11602]: 2022-09-19 12:57:30.686 DEBUG (MainThread) [custom_components.xplora_watch.coordinator] Finished fetching xplora_watch-34051 data in 2.686 seconds (success: True)

Heres a picture, how it looks on my UI:

That's how it looks on UI:

Person with device_tracker attached (always displays unknown)
device_tracker entity directly shown
Person with another (working) device_tracker attached (google maps)
Google maps device tracker directly shown

Hi, this is not a bug, just not implemented


Add user search

[!WARNING]
This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

Current dependencies on/for this PR:

main

PR #3

PR #4

PR #5   👈

This stack of pull requests is managed by Graphite.


bump up timeout for travis

Even if this burns roughly 100 extra seconds (4 failed tests * 25 seconds extra per test), that is vastly less than every time this timeout kicks off and we click 'reload' on a PR and we suck up a queue slot for another 20 minutes or so...
Maybe there's a better fix, but this is better than clicking reload...

:+1:

:+1:

File a bug on it sometimes being super slow?
Also, might not fix it--it could be the issue is the server not starting at all.

I figure the fastest way to find out if the server isn't starting is to merge this...

(i'd also argue that this patch is correct anyway -- we should always be patient before timing out tests and forcing the user to click reload in the travis UI which is horribly expensive)

lol, dammit wrong branch...


Windows: MSVC doesn't define ssize_t

Thanks ! Could you please also add that code to fuzz/main.c !?
And what do you think about
#if defined _MSC_VER && ! defined ssize_t
...

?
Just in case a newer version of MSVC has a ssize_t defined. Or is it too theoretical ?

Coverage remained the same at 93.625% when pulling 6c71799b443e0e88f02a121a28fd62d19746e6bf on xhaakon:master into 19359c673397ddc1d8246eacea06a053146b1ef8 on rockdaboot:master.

Hi,

Thanks ! Could you please also add that code to fuzz/main.c !?

Added.

#if defined _MSC_VER && ! defined ssize_t

Yes, that makes sense. Changed in both locations.

Thanks for that !


Adds metadata

The Metadata fields themselves
Scatter/gather for Global DeployConfigs
and sous metadata get
Hoping to add set before EOD

Current coverage is 60.69% (diff: 72.76%)

No coverage report found for master at 0f31c3d.

Powered by Codecov. Last update 0f31c3d...22da180


WORK IN PROGRESS: Explicitly delete c-ares fd's from pollset set after c-ares is done

In all of the existing tests for c-ares right now, the driving pollset set is quickly and completely unref'd and destroyed as soon as the test is over (by the channel). Google default credentials introduces a special case in that the google default credentials object is created as a global and potentially left around for a long time -  it's user receives a second ref of it. It appears that the OAuth fetcher within google default credentials effectively refs that driving pollset set but then doesn't let that ref go until it's destroyed, and as a result when c-ares is used with google default creds, the fd's created by c-ares are added to driving pollset set are not unref'd in a timely manner.
This test is a little messy but it seems to be a minimal way of catching the earlier bug.
(as a slightly separate issue, I'm having a hard time telling when g_default_credentials is fully unref'd - it seems to need grpc_flush_cached_google_default_credentials to be called?).

test needs more investigation for poll and poll-cv failures

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%     +99 [None]                                                                              +339  +0.0%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +15%      +7 [Unmapped]                                                                            +7   +15%
  +3.6%     +13 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +13  +3.6%
       +13%     +13 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +13   +13%
      +4.0%      +4 grpc_core::NewGrpcPolledFdLocked                                                      +4  +4.0%

  +0.0%    +160 TOTAL                                                                               +400  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] Performance differences noted:
Benchmark                                                                cpu_time    real_time
-----------------------------------------------------------------------  ----------  -----------
BM_PumpStreamServerToClient<InProcess>/262144                            +7%         +7%
BM_StreamingPingPong<InProcess, NoOpMutator, NoOpMutator>/262144/1       -4%         -4%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/32768      -4%         -4%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/262144  -4%         -4%
BM_UnaryPingPong<InProcess, NoOpMutator, NoOpMutator>/0/2097152          +4%         +4%
BM_UnaryPingPong<InProcess, NoOpMutator, NoOpMutator>/0/262144           +5%         +5%
BM_UnaryPingPong<InProcess, NoOpMutator, NoOpMutator>/262144/262144      +8%         +8%
BM_UnaryPingPong<MinInProcess, NoOpMutator, NoOpMutator>/0/262144        +4%         +4%
BM_UnaryPingPong<MinInProcess, NoOpMutator, NoOpMutator>/262144/262144   +9%         +9%

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%     +99 [None]                                                                              +339  +0.0%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +15%      +7 [Unmapped]                                                                            +7   +15%
  +3.6%     +13 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +13  +3.6%
       +13%     +13 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +13   +13%
      +4.0%      +4 grpc_core::NewGrpcPolledFdLocked                                                      +4  +4.0%

  +0.0%    +160 TOTAL                                                                               +400  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%    +106 [None]                                                                              +394  +0.0%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +15%      +7 [Unmapped]                                                                            +7   +15%
   +11%     +38 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +38   +11%
       +23%     +22 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +22   +23%
       +22%     +22 grpc_core::NewGrpcPolledFdLocked                                                     +22   +22%

  +0.0%    +192 TOTAL                                                                               +480  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] Performance differences noted:
Benchmark                                                                              cpu_time    real_time
-------------------------------------------------------------------------------------  ----------  -----------
BM_StreamingPingPong<MinInProcess, NoOpMutator, NoOpMutator>/262144/1                  -4%         -4%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/262144                   -4%         -4%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/32768                 -6%         -6%
BM_StreamingPingPongWithCoalescingApi<InProcess, NoOpMutator, NoOpMutator>/262144/1/0  -4%         -4%
BM_StreamingPingPongWithCoalescingApi<InProcess, NoOpMutator, NoOpMutator>/262144/2/0  -6%         -6%
BM_UnaryPingPong<InProcess, NoOpMutator, NoOpMutator>/0/262144                         +6%         +6%

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%     +99 [None]                                                                              +331  +0.0%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +15%      +7 [Unmapped]                                                                            +7   +15%
  +3.6%     +13 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +13  +3.6%
       +13%     +13 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +13   +13%
      +5.0%      +5 grpc_core::NewGrpcPolledFdLocked                                                      +5  +5.0%

  +0.0%    +160 TOTAL                                                                               +392  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[microbenchmarks] No significant performance differences

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%    +106 [None]                                                                              +394  +0.0%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +15%      +7 [Unmapped]                                                                            +7   +15%
   +11%     +38 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +38   +11%
       +23%     +22 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +22   +23%
       +22%     +22 grpc_core::NewGrpcPolledFdLocked                                                     +22   +22%

  +0.0%    +192 TOTAL                                                                               +480  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

It looks like this approach involves creating a new pollset_set for each individual fd, and there may be several fds per request.  Please check with @sreecha about whether this is reasonable -- in particular, I'm not sure how much overhead is involved in creating and destroying pollset_sets.  I also seem to recall that there are some weird, counter-intuitive semantics surrounding removing pollset_sets that have been added to other pollset_sets.
If there's significant overhead, then an alternative might be to create a pollset_set in the ev driver and use that for all of the fds.  This would at least reduce the overhead to one pollset_set per request, rather than one per fd.  And I think it would eliminate the lifetime issues, since IIRC (but correct me if I'm wrong) the ev driver lives until after all of the fds are destroyed.

Just a note that in general, I think that in a common case (certainly this is the case in our tests), the ares event drivers are usually going to be one-to-one with a single fd (since more fd's are only created if a new DNS server is contact and/or TCP is used).

If there's significant overhead, then an alternative might be to create a pollset_set in the ev driver and use that for all of the fds. This would at least reduce the overhead to one pollset_set per request, rather than one per fd. And I think it would eliminate the lifetime issues, since IIRC (but correct me if I'm wrong) the ev driver lives until after all of the fds are destroyed.

Ack this would sound good to me too.

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%     +83 [None]                                                                              +419  +0.0%
  +2.1%     +48 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.cc       +48  +2.1%
      +6.5%     +24 grpc_ares_ev_driver_create_locked                                                    +24  +6.5%
       +33%     +16 [Unmapped]                                                                           +16   +33%
      +4.7%      +8 grpc_ares_ev_driver_unref                                                             +8  +4.7%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +15%      +7 [Unmapped]                                                                            +7   +15%
  +3.6%     +13 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +13  +3.6%
       +13%     +13 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +13   +13%
      +4.0%      +4 grpc_core::NewGrpcPolledFdLocked                                                      +4  +4.0%

  +0.0%    +192 TOTAL                                                                               +528  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

one more option here might be to restructure things so that the query completion callback isn't invoked until the c-ares event driver (and its fd's) are destroyed, but I think that would need a more involved change.

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%    +243 [None]                                                                              +755  +0.0%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +17%      +7 [Unmapped]                                                                            +7   +17%
  +1.4%     +32 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.cc       +32  +1.4%
      [NEW]    +380 grpc_ares_ev_driver_create_locked                                                   +380  [NEW]
       +24%     +12 [Unmapped]                                                                           +12   +24%
      [NEW]      +9 grpc_ares_ev_driver_on_queries_complete_locked                                        +9  [NEW]
      +4.7%      +8 grpc_ares_ev_driver_unref                                                             +8  +4.7%
  +0.3%     +16 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc         +16  +0.3%
      [NEW]    +236 log_address_sorting_list(grpc_lb_addresses*, char const*) [clone .isra.1]           +236  [NEW]
      [NEW]     +73 grpc_ares_complete_request_locked                                                    +73  [NEW]
      +2.7%     +24 on_hostbyname_done_locked                                                            +24  +2.7%
       +15%     +18 [Unmapped]                                                                           +18   +15%
      +1.5%      +8 on_srv_query_done_locked                                                              +8  +1.5%
      +0.4%      +6 grpc_dns_lookup_ares_locked_impl                                                      +6  +0.4%
  +3.6%     +13 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +13  +3.6%
       +13%     +13 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +13   +13%
      +4.0%      +4 grpc_core::NewGrpcPolledFdLocked                                                      +4  +4.0%

  +0.0%    +352 TOTAL                                                                               +864  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%    +243 [None]                                                                              +739  +0.0%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +17%      +7 [Unmapped]                                                                            +7   +17%
  +1.4%     +32 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.cc       +32  +1.4%
      [NEW]    +380 grpc_ares_ev_driver_create_locked                                                   +380  [NEW]
       +24%     +12 [Unmapped]                                                                           +12   +24%
      [NEW]      +9 grpc_ares_ev_driver_on_queries_complete_locked                                        +9  [NEW]
      +4.7%      +8 grpc_ares_ev_driver_unref                                                             +8  +4.7%
  +0.3%     +16 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc         +16  +0.3%
      [NEW]    +236 log_address_sorting_list(grpc_lb_addresses*, char const*) [clone .isra.1]           +236  [NEW]
      [NEW]     +73 grpc_ares_complete_request_locked                                                    +73  [NEW]
      +2.7%     +24 on_hostbyname_done_locked                                                            +24  +2.7%
       +15%     +18 [Unmapped]                                                                           +18   +15%
      +1.5%      +8 on_srv_query_done_locked                                                              +8  +1.5%
      +0.4%      +6 grpc_dns_lookup_ares_locked_impl                                                      +6  +0.4%
  +3.6%     +13 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +13  +3.6%
       +13%     +13 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +13   +13%
      +4.0%      +4 grpc_core::NewGrpcPolledFdLocked                                                      +4  +4.0%

  +0.0%    +352 TOTAL                                                                               +848  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

[microbenchmarks] No significant performance differences

Looking at the new integration between the request and EV driver objects, I think this is really screaming out for the two to be combined into a single object.  I won't insist upon doing that in this PR, but I think we should do that before C++-ifying this code, because it will be really hard to represent the current relationship between these objects using any of our C++ ref-counting classes.

Reviewed 2 of 4 files at r1, 1 of 3 files at r5, 1 of 2 files at r6, 4 of 4 files at r7.
Review status: all files reviewed, 3 unresolved discussions (waiting on @apolcyn, @dgquintas, @yihuazhang, @sreecha, and @AspirinSJL)

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 147 at r7 (raw file):

}

static void grpc_ares_request_increment_pending_queries_locked(

I think these functions could still be called ref and unref.  The fact that the underlying type is a size_t instead of a gpr_refcount is an implementation detail; the external functionality is still the same here.

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 416 at r7 (raw file):

  r->error = GRPC_ERROR_NONE;
  r->pending_queries = 0;
  error = grpc_ares_ev_driver_create_locked(&r->ev_driver, interested_parties,

Why was this moved?
This seems incorrect to me.  The goto error_cleanup results in us returning nullptr, which means that we're not returning r, but we are not deallocating it anywhere.  It looks like this would result in a memory leak upon error.

src/core/lib/iomgr/iomgr.h, line 39 at r7 (raw file):

/* Exposed only for testing */
size_t grpc_iomgr_count_objects_for_testing();

Instead of exposing this, would it be sufficient to run the test with the environment variable GRPC_ABORT_ON_LEAKS=1?

Comments from Reviewable

[trickle] No significant performance differences

Review status: all files reviewed, 3 unresolved discussions (waiting on @apolcyn, @markdroth, @dgquintas, @yihuazhang, @sreecha, and @AspirinSJL)

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 147 at r7 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
I think these functions could still be called ref and unref.  The fact that the underlying type is a size_t instead of a gpr_refcount is an implementation detail; the external functionality is still the same here.

Done.

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 416 at r7 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Why was this moved?
This seems incorrect to me.  The goto error_cleanup results in us returning nullptr, which means that we're not returning r, but we are not deallocating it anywhere.  It looks like this would result in a memory leak upon error.

I needed to move grpc_ares_ev_driver_create_locked below the creation of the request object only because this PR changes grpc_ares_ev_driver_create_locked to take the request object as a param.
But good point, I've added a gpr_free(r) into the error_cleanup label  for this.

src/core/lib/iomgr/iomgr.h, line 39 at r7 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Instead of exposing this, would it be sufficient to run the test with the environment variable GRPC_ABORT_ON_LEAKS=1?

Unfortunately I don't think it can help here. As far as I can see, GRPC_ABORT_ON_LEAKS can only  catch the case in which an fd is leaked even after all grpc objects are destroyed and grpc_shutdown is called. The new test in this PR is concerned not so much about if c-ares fd's are cleaned up, but instead when (currently, c-ares fd's are getting destroyed and cleaned up properly after all pollset sets that they've been added to are destroyed - but we need to make sure to destroy and free them as soon as they are no longer in use, otherwise we're potentially hanging on to them over the process lifetime).

Comments from Reviewable

[microbenchmarks] No significant performance differences

Reviewed 1 of 1 files at r8.
Review status: all files reviewed, 1 unresolved discussion (waiting on @markdroth, @dgquintas, @yihuazhang, @sreecha, and @AspirinSJL)

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 416 at r7 (raw file):
Previously, apolcyn wrote…
I needed to move grpc_ares_ev_driver_create_locked below the creation of the request object only because this PR changes grpc_ares_ev_driver_create_locked to take the request object as a param.
But good point, I've added a gpr_free(r) into the error_cleanup label  for this.

I think there's still a bug here.    So adding a gpr_free(r) after error_cleanup: means that we will free it twice in those cases.
I suggest removing the ocurrences of gpr_free(r) that occur right before goto error_cleanup.

Comments from Reviewable

Ack on combining the request and driver objects into one. I'll leave that for a separate PR if though if sounds good

Review status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn, @dgquintas, @yihuazhang, @sreecha, and @AspirinSJL)

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 416 at r7 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
I think there's still a bug here.    So adding a gpr_free(r) after error_cleanup: means that we will free it twice in those cases.
I suggest removing the ocurrences of gpr_free(r) that occur right before goto error_cleanup.

Whoops, done

Comments from Reviewable

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%    +227 [None]                                                                           +4.65Ki  +0.1%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +17%      +7 [Unmapped]                                                                            +7   +17%
  +1.4%     +32 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.cc       +32  +1.4%
      [NEW]    +380 grpc_ares_ev_driver_create_locked                                                   +380  [NEW]
       +24%     +12 [Unmapped]                                                                           +12   +24%
      [NEW]      +9 grpc_ares_ev_driver_on_queries_complete_locked                                        +9  [NEW]
      +4.7%      +8 grpc_ares_ev_driver_unref                                                             +8  +4.7%
  +3.6%     +13 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +13  +3.6%
       +13%     +13 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +13   +13%
      +4.0%      +4 grpc_core::NewGrpcPolledFdLocked                                                      +4  +4.0%

  +0.0%    +320 TOTAL                                                                            +4.74Ki  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

Combining the objects in a separate PR sounds fine.  Thanks!

Reviewed 1 of 1 files at r9.
Review status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dgquintas, @yihuazhang, @sreecha, and @AspirinSJL)

Comments from Reviewable

squashed down commits

****************************************************************

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +0.0%    +259 [None]                                                                           +4.65Ki  +0.1%
  +3.3%     +48 src/core/lib/iomgr/iomgr.cc                                                          +48  +3.3%
      [NEW]     +41 grpc_iomgr_count_objects_for_testing                                                 +41  [NEW]
       +17%      +7 [Unmapped]                                                                            +7   +17%
  +1.4%     +32 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.cc       +32  +1.4%
      [NEW]    +380 grpc_ares_ev_driver_create_locked                                                   +380  [NEW]
       +24%     +12 [Unmapped]                                                                           +12   +24%
      [NEW]      +9 grpc_ares_ev_driver_on_queries_complete_locked                                        +9  [NEW]
      +4.7%      +8 grpc_ares_ev_driver_unref                                                             +8  +4.7%
  +3.6%     +13 src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posi     +13  +3.6%
       +13%     +13 grpc_core::GrpcPolledFdPosix::~GrpcPolledFdPosix                                     +13   +13%
      +4.0%      +4 grpc_core::NewGrpcPolledFdLocked                                                      +4  +4.0%

  +0.0%    +352 TOTAL                                                                            +4.74Ki  +0.0%

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

[microbenchmarks] No significant performance differences

bazel test failures: https://github.com/grpc/grpc/issues/14240


Partitioning Data Set for k-fold analysis

Unless we want to do the kfold by hand, it might be a good idea to cut the data input (once it is loaded into the array) into 5 parts. That way we can do 5fold cross validation automatically, running each 4/5 training set until convergence and then testing it using each 1/5 test set. I guess this could also be done by hand, but then we would need a "test" option where we load test data (otherwise the machine is just going to load it like another training set and go to town rather than keeping state and outputting predictions).

Because the data is in an array of arraylists (i.e. Data = ArrayList[] inputVector;) you could just cut the inputVector arrays into 5 parts by dividing the inputVector.length by 5. Then alternate training on each of the different sections until convergence and then calculate the error on the training set. After that you just average the 5 different training set error calculations and you have the  average error across all of the trials for 5-fold cross validation (and you don't have to do it by hand).


Add "view latest consultation" or "Download Latest Discharge Summary" button on discharged patient

Is your feature request related to a problem? Please describe.
Currently, users are facing issues with accessing and downloading discharge summaries. previously, the default page opened for discharged patients was the patient dashboard/consultation page itself. now users have to click on encounter from patient details page and then go to discharge summary.
its better to avail "View latest consultation" or "Download latest discharge summary" button on discharged patients details page.
Describe the solution you'd like

[ ] Add View latest consultation on discharged patients details page or,
[ ] Add Download latest discharge summary button

Reported by Hub engineers on tech review calls
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context or screenshots about the feature request here.

Can I work on this issue? @samholics

I would like to work on this

Will revisit the issue once rearchitecturing is done


Store Jobs in JobSet by Queue:JobSet Pair: add storage

We currently store a mapping from JobSet -> set of JobIds in that JobSet in Redis in Armada.
This however means that if multiple Queues have the same JobSet name, then Jobs from different Queues will be stored under the same key in Redis.
As such we want to make sure that the key is a Queue:JobSet pair instead. This would make JobCancellation easier, as in order to cancel all Jobs in a JobSet, we currently have to load all Job ids in a queue and filter out those in the specified JobSet.
This is the first step of the migration to avoid breaking existing setups, which allows for storing Job ids in JobSets using both methods.

Wouldn't it be better to hash the queue and job set name together, i.e., to use as the key
key = sha1(sha1(queue name) + sha1(job set name))?
It's safer (since we're won't have issues like a + ab = aa + b without needing to disallow particular characters), it means keys will be shorter (a fixed 160 bits if using sha1), and it improves string comparison performance (since keys won't have long common prefixes).
We use this method job deduplication:
https://github.com/G-Research/armada/blob/master/internal/armada/server/submit_to_log.go#L103


Custom key comparison functions

This PR adds some new method for database opening to let the user specify a custom key comparison function if necessary and fixes #86.

[x] Expose an API to create/open databases with a custom key comparison function.
[x] Ensure that a database correctly uses a comparison function if it has been open with one.
[ ] Expose a safe Rust API for the user (instead of an extern "C" one).

Expose a Rust API instead of the LMDB required function signature
The current exposed API for the custom key comparison function is an ugly extern "C" function, I would like to let the user give a real Rust function instead. It is ugly in the sense that I don't want to expose the MDB_val type (a struct with start and length fields) and therefore the from_val function (transform an MDB_val into a Rust slice).
// What the API user must define itself, we give that to LMDB directly.
extern "C" fn my_custom_key_cmp(a: *const MDB_val, b: *const MDB_val) -> i32;

// What I would like to ask the user to give and wrap.
fn my_custom_key_cmp(a: &[u8], b: &[u8]) -> Ordering;

// The wrapper that could be used to call the user defined function,
// transforming a C call into a Rust call, invisible to the API user.
extern "C" fn wrapper(a: *const MDB_val, b: *const MDB_val) -> i32 {
    // transform a and b into Rust slices here using `from_val`.
    // PROBLEM how can I pass the user function here? There is no easy way.
    match (user_cmp_fn)(a, b) {
        Ordering::Less => -1,
        Ordering::Equal => 0,
        Ordering::Greater => 1,
    }
}

As it is not quite possible to pass the user-defined function to a wrapper function with the LMDB required signature, I thought that we could maybe be able to "generate" a function with the required signature that is monomorphized to call the user-defined function (no hidden parameter), but is it possible to do that, I am not sure, I tried by using a closure and a Box::into_raw call, but it didn't work well (dyn Fn can't be cast into extern "C" fn).

Perfect solution: ask LMDB to change the signature to add a void *user_data that would be passed to the comparator.
Hack: pass the rust function via a thread_local? :D
Keep in mind: https://matklad.github.io/2020/10/03/fast-thread-locals-in-rust.html

Perfect solution: ask LMDB to change the signature to add a void *user_data that would be passed to the comparator.
Hack: pass the rust function via a thread_local? :D
Keep in mind: https://matklad.github.io/2020/10/03/fast-thread-locals-in-rust.html

extern "C" fn wrapper<C: CustomKeyCmp>

Yeah, for just static functions (not closures) this is perfect, and if users want to capture any state, they have to do global passing themselves

extern "C" fn wrapper<C: CustomKeyCmp>

Yeah, for just static functions (not closures) this is perfect, and if users want to capture any state, they have to do global passing themselves

Thank you for the help @myfreeweb,
Indeed it is not easy to accept a user-defined Rust closure as the custom key comparison function. A lot of people helped me on Reddit and some of them were proposing this solution (using a trait as a generic parameter to an extern "C" function) therefore generating the appropriate function for LMDB.

Yeah, for just static functions (not closures) this is perfect, and if users want to capture any state, they have to do global passing themselves

And yes, this can be a good enough solution for those who really need a Rust closure with a context or something.

Thank you for the help @myfreeweb,
Indeed it is not easy to accept a user-defined Rust closure as the custom key comparison function. A lot of people helped me on Reddit and some of them were proposing this solution (using a trait as a generic parameter to an extern "C" function) therefore generating the appropriate function for LMDB.

Yeah, for just static functions (not closures) this is perfect, and if users want to capture any state, they have to do global passing themselves

And yes, this can be a good enough solution for those who really need a Rust closure with a context or something.

The complexity is more there than what I thought, introducing a custom key comparison function to the database (that is in fact based on the LMDB one) introduces a lot of little behavior like the fact that we can't just advance and retreat a key by only using the bytes of it as it is not necessarily the way it works (think about integer in little-endian).
So I am closing this now as I don't want to take more time on that problem for now.

The complexity is more there than what I thought, introducing a custom key comparison function to the database (that is in fact based on the LMDB one) introduces a lot of little behavior like the fact that we can't just advance and retreat a key by only using the bytes of it as it is not necessarily the way it works (think about integer in little-endian).
So I am closing this now as I don't want to take more time on that problem for now.


Clean up operator code

Currently most of the operator code is unused and generated by kubebuilder.  We should remove anything we are not using and structure the operator is a way that is more readable.

operator should have a pkg folder with the source
main.go to run the operator
go mod
anything that is specific for kubebuilder goes in pkg/sdk
anything not needed for building the operator docker file is deleted

we discussed and agreed to move the operator to top level

version: 255eabe
structure aligns with what have discussed and is easy to understand.


Write bash script that enables users to enter key details necessary to load data into Redshift

Those details need to include:

AWS access key
AWS secret key
Bucket on S3 to load data to
Redshift host
Redshift port
Redshift database
Redshift schema
Redshift username
Redshift password

No longer required as we will be loading the Postgres on the box.