Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not reuse the same timeout during connection timeout #72

Conversation

enidgjoleka
Copy link
Contributor

@enidgjoleka enidgjoleka commented Mar 14, 2020

Hi,

This PR tries to address two points:

  • kpro_connection:start/3 spawns a new process which calls init/4. This fun was using the same connection_timeout during different phases of establishing the connection.
    This PR tries to use the "remaining timeout" in the different phases.
  • kpro_brokers:discover_partition_leader/4 was calling kpro_lib:ok_pipe/1 without a timeout. It is true that now there is only one function that does network calls, kpro_connection:request_sync/3 but better be safe.

Please let me know if I missed something!

@enidgjoleka
Copy link
Contributor Author

There is a failing test case which I am not sure is related to my change:

in call from kpro_txn_tests:test_txn_produce_2/2 (/home/travis/build/klarna/kafka_protocol/test/kpro_txn_tests.erl, line 164)
**error:{badmatch,{error,[{<<"test-topic">>,0,concurrent_transactions}]}}
  output:<<"">>

@enidgjoleka enidgjoleka force-pushed the do-not-reuse-the-same-timeout-during-connection-timeout branch from 228acba to 4cb069e Compare April 12, 2020 16:10
@enidgjoleka
Copy link
Contributor Author

All the tests pass in both in OTP-21/OTP-22 in my local machine and I can not seem to reproduce this test failure.

It would be nice if I could get some traction on this PR.

@CLAassistant
Copy link

CLAassistant commented Jan 21, 2021

CLA assistant check
All committers have signed the CLA.

@zmstone
Copy link
Contributor

zmstone commented Feb 23, 2021

why isn't this merged yet.

@enidgjoleka
Copy link
Contributor Author

@zmstone to be honest I don't know, maybe because there is a failing test case.

@robertoaloi
Copy link
Contributor

I think @enidgjoleka is correct:

module 'kpro_txn_tests'
  kpro_txn_tests: txn_produce_test...[2.297 s] ok
  kpro_txn_tests: txn_fetch_produce_test...[0.126 s] ok
  kpro_txn_tests: txn_produce_2_tx_test...*failed*
in function kpro_txn_tests:'-test_txn_produce_2/2-fun-2-'/6 (/home/travis/build/klarna/kafka_protocol/test/kpro_txn_tests.erl, line 155)
in call from kpro_txn_tests:test_txn_produce_2/2 (/home/travis/build/klarna/kafka_protocol/test/kpro_txn_tests.erl, line 164)
in call from eunit_test:'-mf_wrapper/2-fun-0-'/2 (eunit_test.erl, line 273)
in call from eunit_test:run_testfun/1 (eunit_test.erl, line 71)
in call from eunit_proc:run_test/1 (eunit_proc.erl, line 510)
in call from eunit_proc:with_timeout/3 (eunit_proc.erl, line 335)
in call from eunit_proc:handle_test/2 (eunit_proc.erl, line 493)
in call from eunit_proc:tests_inorder/3 (eunit_proc.erl, line 435)
**error:{badmatch,{error,[{<<"test-topic">>,0,concurrent_transactions}]}}
  output:<<"">>
  [done in 2.540 s]
=======================================================
  Failed: 1.  Skipped: 0.  Passed: 55.
===> Error running tests
Makefile:39: recipe for target 'eunit' failed
make: *** [eunit] Error 1

@zmstone
Copy link
Contributor

zmstone commented Feb 24, 2021

likely caused by inter test case race condition.
will follow up with another issue which should not be a blocker for this PR.

@zmstone zmstone changed the base branch from master to api-timeout-and-deadline July 11, 2021 09:35
@zmstone zmstone merged commit 99845b8 into kafka4beam:api-timeout-and-deadline Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants