Skip to content

Conversation

srkukarni
Copy link
Contributor

Motivation

In the testAutoSchemaFunction integration test, we create a function that uses auto_consume. However the function is submitted before a topic/schema is created. This leads to the function failing to start a few times before a producer comes in and produces the schema/messages.

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@srkukarni srkukarni added this to the 2.4.0 milestone Mar 27, 2019
@srkukarni srkukarni self-assigned this Mar 27, 2019
@srkukarni srkukarni requested review from sijie and jerrypeng March 27, 2019 01:10
@srkukarni
Copy link
Contributor Author

run java8 tests

getFunctionStatus(functionName, numMessages);
// get function status. Note that this function might restart a few times until
// the producer above writes the messages.
getFunctionStatus(functionName, numMessages, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why will this restart a couple of times? That doesn't sound like the correct behavior

Copy link
Contributor

@jerrypeng jerrypeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its unclear why the correct behavior for that function is to restart. Are we sure we are not rewritting the test to accommodate incorrect behavior?

@srkukarni
Copy link
Contributor Author

@jerrypeng currently the way things are plumed inside the client, the schema has to be established at consumer create time before proceeding. Changing this to waiting for the first message before trying to lookup is possible, but I suspect is pretty extensive change. @sijie thoughts?

@sijie
Copy link
Member

sijie commented Mar 28, 2019

@srkukarni @jerrypeng I think current change is the easiest way to fix the integration tests. alternatively we can create the topic prior to doing any tests.

@jerrypeng
Copy link
Contributor

@sijie @srkukarni should we have a discussion on what the correct behavior of auto consume consumer when there is no topic because the current behavior and the error message it produces is very confusing and not helpful to the user.

@sijie
Copy link
Member

sijie commented Mar 28, 2019

I think if there is no topic, it should fail immediately since auto consume requires topic exists first. That’s the expected behavior. We can change the logging to be more explicit but I don’t think we should change the behavior.

@srkukarni srkukarni merged commit 70cc682 into apache:master Mar 28, 2019
@srkukarni srkukarni deleted the fix_integration_test branch March 28, 2019 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants