Skip to content

Conversation

@wuchong
Copy link
Member

@wuchong wuchong commented Feb 25, 2016

  1. Move validate-kryo-conf-basic and validate-kryo-conf-fail to TestConfigValidate.java, and merge this two tests into one
  2. I'm not sure wether to translate test-clojure-serialization. So I translate it and commet it.

import com.google.common.collect.ImmutableMap;
import org.apache.storm.utils.Utils;
import org.apache.storm.validation.ConfigValidation;
import org.apache.storm.validation.ConfigValidation.*;

Choose a reason for hiding this comment

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

please change * import

Choose a reason for hiding this comment

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

never mind, old code.

@wuchong
Copy link
Member Author

wuchong commented Feb 26, 2016

@abhishekagarwal87 Thanks for your hint.

I have moved test-clojure-serialization back to serialization-test.clj, and squashed the commits.

@abhishekagarwal87
Copy link
Contributor

Thanks @wuchong Looks good to me.

@wuchong
Copy link
Member Author

wuchong commented Mar 4, 2016

@revans2 can you have a look ?

List ret = null;
try {
ret = deserialize(serialize(vals, conf), conf);
} catch (IOException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have the IOException thrown by roundtrip. Catching it will still cause the test to fail, but it will be harder to debug.

@revans2
Copy link
Contributor

revans2 commented Mar 4, 2016

Just one minor comment. I am +1 even without the update.

@wuchong
Copy link
Member Author

wuchong commented Mar 6, 2016

@revans2 addressed

@harshach
Copy link
Contributor

+1

@asfgit asfgit merged commit cdc041e into apache:master Mar 13, 2016
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.

6 participants