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.

Sign up for GitHub

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

Jump to bottom

Fixes #31 - errors during kafka deserializer (passing) test execution #25

Merged
merged 1 commit into from
Jan 7, 2019
Merged

Fixes #31 - errors during kafka deserializer (passing) test execution #25

merged 1 commit into from
Jan 7, 2019

Conversation

baskaranz
Copy link
Contributor

This fixes the error being thrown by the test, which breaks the build, although the test passes.

@woop
Copy link
Member

woop commented Jan 3, 2019

@baskaranz can you please squash the commits and also create an issue for this PR?

@feast-ci-bot feast-ci-bot added size/M and removed size/L labels Jan 3, 2019
@baskaranz baskaranz changed the title fix for kafka feature row deserializer test throwing error Fix for errors during kafka deserializer (passing) test execution #31 Jan 3, 2019
@baskaranz baskaranz changed the title Fix for errors during kafka deserializer (passing) test execution #31 Fixes #31 - errors during kafka deserializer (passing) test execution Jan 3, 2019
@tims
Copy link
Contributor

tims commented Jan 3, 2019 •
edited
Loading

I'd be interested to know why this fixed the warnings?

Also for reference, I think you need to link to the issue in a comment, not the title for it to connect them properly. eg: #31

tims
tims suggested changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@tims tims left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you make sure you are using the google-java-format plugin and reorganise imports and format the code?

you seem to have different import patterns. If you are using the google-java-format plugin it would also keep ContextConfurgation at the bottom.

@baskaranz
Copy link
Contributor Author

baskaranz commented Jan 3, 2019 •
edited
Loading

@tims I've used google-java-format plugin and it didn't reformat my code to keep ContextConfiguration at the bottom.

Coming to the fix, I've seen multiple issue raised even with spring and other forums about the same issue, which stated that it fails their jenkins build etc., (assuming we would face the same as well) Here's one

spring-projects/spring-kafka#194

I've used @ClassRule for KafkaEmbedded instead of bean and it resolved the ERROR messages I see while running the test as suggested and also referring the below one

https://github.com/spring-projects/spring-kafka/blob/master/src/reference/asciidoc/testing.adoc

@tims
Copy link
Contributor

tims commented Jan 3, 2019

Make sure it's enabled for your project, you need turn it on, not just install it.

@baskaranz
Copy link
Contributor Author

@tims Intelij was smart enough to suggest me when I installed the plugin to enable for my projects and I did, not sure why it doesn't reorganize the code when it reformats :)

@tims
Copy link
Contributor

tims commented Jan 3, 2019

Sorry to keep harping on about it, but formatting changes in every commit will continue to drive us nuts.

I've found I need to enable for every project separately, it's not global, so you may have enabled for another project?

Can you check preferences -> other settings -> google-java-format settings
And ensure Enable google-java-format is checked.
And make sure you've restarted intellij since installing.

Then run cmd-alt-shift-L in the file (assuming on osx)

@woop
Copy link
Member

woop commented Jan 3, 2019

Sorry to keep harping on about it, but formatting changes in every commit will continue to drive us nuts.

I've found I need to enable for every project separately, it's not global, so you may have enabled for another project?

Can you check preferences -> other settings -> google-java-format settings
And ensure Enable google-java-format is checked.
And make sure you've restarted intellij since installing.

Then run cmd-alt-shift-L in the file (assuming on osx)

Should we make an issue for creating a contributing document again? Where we can specify how your development environment should be set up for contributing to each component?

@tims
Copy link
Contributor

tims commented Jan 3, 2019

yep I'll make one

@feast-ci-bot feast-ci-bot added size/L and removed size/M labels Jan 3, 2019
@baskaranz
Copy link
Contributor Author

@tims change request addressed

@woop
Copy link
Member

woop commented Jan 3, 2019 •
edited
Loading

@baskaranz you mind squashing this into atomic commits? Actually, with Prow we should be able to auto-squash in the future, but for now it is manual.

@woop
Copy link
Member

woop commented Jan 4, 2019

/retest
/assign tims
/assign pradithya

@woop
Copy link
Member

woop commented Jan 4, 2019

The test wont pass until a rebase is done on current master. Also, @pradithya can you please review this instead of me? You have more familiarity.

@pradithya
Copy link
Collaborator

@baskaranz Is it possible to remove dependency to org.springframework.kafka for testing in the future?

@pradithya
Copy link
Collaborator

pradithya commented Jan 4, 2019 •
edited
Loading

@baskaranz @tims the import ordering is not handled by google-java-format plugin, you'll need to import google code style for intelliJ to your code style settings.
It seems in latest commit the import ordering is still incorrect.

@feast-ci-bot feast-ci-bot added size/M and removed size/L labels Jan 6, 2019
@tims
Copy link
Contributor

tims commented Jan 6, 2019

/lgtm

@pradithya since org.springframework.kafka snuck in with a previous pull request, I'm okay if we make
a new issue to remove it. But I'll defer to you.

@pradithya
Copy link
Collaborator

/approve

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pradithya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot feast-ci-bot merged commit a59813f into feast-dev:master Jan 7, 2019
Yanson pushed a commit to Yanson/feast that referenced this pull request Jul 29, 2020
@algattik
Terraform CI/CD env (feast-dev#25)
dad9ffe 
Closes KE-712, KE-738

Deploys an IaC environment with Feast and Databricks, and runs e2e tests with Redis (using DirectRunner for now, as tests with Databricks runner depend on PR feast-dev#19 and currently fail with timeout issues)

Deletes infra/charts/feast/charts/feast-serving/templates/configmap-store.yaml (To be reviewed)Closes KE-712, KE-738

Deploys an IaC environment with Feast and Databricks, and runs e2e tests with Redis (using DirectRunner for now, as tests with Databricks runner depend on PR feast-dev#19 and currently fail with timeout issues)

Deletes infra/charts/feast/charts/feast-serving/templates/configmap-store.yaml
dmartinol pushed a commit to dmartinol/feast that referenced this pull request Jul 5, 2024
@redhatHameed
Merge pull request feast-dev#25 from redhatHameed/server-permissions
7d6c36b 
Added permission assert check for registry server, offline server, online server functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tims tims tims approved these changes

Assignees

@tims tims

@pradithya pradithya

Labels
approved lgtm size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@baskaranz @woop @tims @pradithya @feast-ci-bot