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

STORM-2578: Apply new code style to storm-elasticsearch #2803

Merged
merged 1 commit into from
Aug 20, 2018
Merged

STORM-2578: Apply new code style to storm-elasticsearch #2803

merged 1 commit into from
Aug 20, 2018

Conversation

milantracy
Copy link
Contributor

No description provided.

@milantracy
Copy link
Contributor Author

@HeartSaVioR could you review the changes? Thanks.

String index = tupleMapper.getIndex(tuple);
String type = tupleMapper.getType(tuple);
String id = tupleMapper.getId(tuple);
Map<String, String> params = tupleMapper.getParams(tuple, new HashMap<>());

Choose a reason for hiding this comment

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

should indent with Tab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi Danny, we might be not allowed to indent with tab because of FileTabCharacter in checkstyle. Please correct me if I misunderstood here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was broken indentation (mixed tab and space) and the change fixes the indentation.

Copy link

@danny0405 danny0405 left a comment

Choose a reason for hiding this comment

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

Looks good broadly, then what about the checkstyle config now, did it not work for the module now?

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Aug 16, 2018

@milantracy Thanks for the contribution! Could you please reduce the number of max violation count in pom.xml in storm-elasticsearch so that we can see how many spots your patch address, and also we never break it again?

@milantracy
Copy link
Contributor Author

Hi @HeartSaVioR , reduced the value to 0.

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

Mostly looks great. Couple of nits. Thanks for the contribution!

*/
public interface EsLookupResultOutput extends Serializable {

/**
* Covert Elasticsearch response to a collection of {@link Values}.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Covert -> Convert

String index = tupleMapper.getIndex(tuple);
String type = tupleMapper.getType(tuple);
String id = tupleMapper.getId(tuple);
Map<String, String> params = tupleMapper.getParams(tuple, new HashMap<>());
Copy link
Contributor

Choose a reason for hiding this comment

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

It was broken indentation (mixed tab and space) and the change fixes the indentation.

*
* @param index index name
* @param type document type to be stored
* @param id unique document id in Elastisearch
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Elastisearch -> Elasticsearch

@milantracy
Copy link
Contributor Author

thanks @HeartSaVioR, change the fix accordingly.

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

+1

Copy link

@danny0405 danny0405 left a comment

Choose a reason for hiding this comment

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

+1

@asfgit asfgit merged commit 8bbee40 into apache:master Aug 20, 2018
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