In D5173#151887, @jon wrote:

In D5173#151704, @max wrote:

I'm checking what's wrong with the Android and iOS builds (maybe the protobuf changes broke some old client-side API code). Requesting review to unblock it for possible comments.

We currently have set(CMAKE_CXX_STANDARD 14) in the CMakeLists.txt file, however, this commit now includes C++17 extensions

The native error messages are related to some native client code that is expecting payload() in the GetResponse. So I should include the changes in this native client code as well according to the proto changes to make it build.

We currently have set(CMAKE_CXX_STANDARD 14) in the CMakeLists.txt file, however, this commit now includes C++17 extensions

Was seeing a lot of:

/workdir/native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp:487:17: warning: decomposition declarations are a C++17 extension [-Wc++17-extensions]
     const auto &[applyMigration, shouldBeInTransaction] = migration;
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warnings.

In D5173#152584, @tomek wrote:

Not really sure about good practices here, but for me it sounds strange that we're using the same function for two completely different purposes. Shouldn't we introduce a new function which handles just the ping?

We already have a unidirectional stream and will switch to the bidirectional stream in the near future. In this case, we can use a single stream to send new messages and pings as well. If we create a new stream request we need to spawn a new additional thread with the request in the client and handle it. This will overcopmlicate the easy thing.

There is an keep-alive gRPC mechanism which maybe we consider to use in the near-term instead of pinging by ourselves. At the moment I've postponed investigating this and created a future task as it doesn't work as expected in the testing repo.

In D5173#152652, @ashoat wrote:

We'll rethink this API in ENG-1072 and ENG-484. Would be good to prioritize ENG-1842 for Tunnelbroker v0.5 as well... I would like us to nail the API down in that version, so we can avoid having to change it while the service is live in the future.

Yes, that's why the current changes are made keeping that in mind to make the transition as easy and fast as possible and not time-consuming.

Why did the builds fail?

In D5173#153043, @ashoat wrote:

Why did the builds fail?

Looks like a Nix configuration issue:

$ trap 'kill -- $$' INT TERM QUIT; cd services/tunnelbroker
rm -rf build && mkdir build && cd build
nix develop --command bash -c "cmake .. && make -j"
./bin/runTests --gtest_filter=-AmqpManager*:DatabaseManager*
path '/opt/homebrew/var/buildkite-agent/builds/mac-broadway-local-1/comm/tunnelbroker-unit-tests/services/tunnelbroker/build' does not contain a 'flake.nix', searching up
do you want to allow configuration setting 'extra-trusted-public-keys' to be set to 'comm.cachix.org-1:70RF31rkmCEhQ9HrXA2uXcpqQKGcUK3TxLJdgcUCaA4=' (y/N)? # Received cancellation signal, interrupting
error: interrupted by the user
🚨 Error: The command exited with status 1
user command error: exit status 1

Probably @max didn't rebase on master after @jon's recent changes. We should make sure the recent changes solve the issue, or otherwise create a task and/or diff to resolve

In D5173#153063, @ashoat wrote:

Probably @max didn't rebase on master after @jon's recent changes.

Yes, that's the problem here.

We should make sure the recent changes solve the issue, or otherwise create a task and/or diff to resolve

Seems that it's solved because the recently rebased diffs are successful. For example: D5081, D5145