----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33488/#review82800 -----------------------------------------------------------
Overall looks good to me. We may need to re-base. checkstyle/import-control.xml <https://reviews.apache.org/r/33488/#comment133624> nit: indentation is not aligned. samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueStore.java <https://reviews.apache.org/r/33488/#comment133626> nit: should have been removed? samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java <https://reviews.apache.org/r/33488/#comment133628> nit: There are still many trailing white spaces. We should remove them. - Yi Pan (Data Infrastructure) On April 27, 2015, 7:59 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33488/ > ----------------------------------------------------------- > > (Updated April 27, 2015, 7:59 p.m.) > > > Review request for samza. > > > Bugs: SAMZA-657 > https://issues.apache.org/jira/browse/SAMZA-657 > > > Repository: samza > > > Description > ------- > > SAMZA-657.v1 > > 1. Add the checkstyle xml files for the following packages (that have Java > code) > > samza-api > samza-core > samza-log4j > samza-kv > samza-test > > 2. Fix some coding style issues found by checkstyle. > > 3. Remove one class EpochPartitioner.java since it is from the old producer > and not used anywhere. > > Some questions I have: > > 1. Current packaging hierarchy seems to me unnecessarily nested and hence the > import-control.xml rules quite messy. For example: > > a. We have a "o.a.s.test" package, with nested "integration.join" etc, but > the test and test-util classes are acutally spread all over other packages, > like "o.a.s.system.mock" > > b. We have a "o.a.s.serializers", and "o.a.s.logging.log4j.serializers". > Shall we just make one "serializers"? > > c. We have both "o.a.s.serializers.model" and "o.a.s.job.model". Shall we > just make one "model"? > > d. We have a top-level "o.a.s.task" and "o.a.s.container.grouper.task", etc.. > > Should we consider make the packaging hierarchy more clearer? > > 2. We are currently claiming to use 2 indentation in order to be consistent > with Scala, while there are some places that 4 indentation are also used. > Shall we just choose one standard and stick with it? The current > checkstyle.xml overrides default (4) to 2. > > > Diffs > ----- > > README.md 7f92020726626e606dbd97b86dcd91f4157c9ea7 > build.gradle 97de3a28f6379e3862eec845da87587b1d4f742e > checkstyle/checkstyle.xml PRE-CREATION > checkstyle/import-control.xml PRE-CREATION > samza-api/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java > 092cb910b40d312217e86420bf1ddfbaf605e9e5 > samza-api/src/main/java/org/apache/samza/config/Config.java > 2b990506864c38ec2c46d55f27c2ba2f98f271ea > samza-api/src/main/java/org/apache/samza/config/MapConfig.java > 38d7424429d4bf81614311c39630b165236d8fbb > samza-api/src/main/java/org/apache/samza/system/SystemStreamPartition.java > 8dcea09ece60f91a890ff6b1abcb4e93c248dfe4 > samza-api/src/main/java/org/apache/samza/util/BlockingEnvelopeMap.java > e30321d521f0fd7d3d69e2858352916142fb27bf > > samza-api/src/main/java/org/apache/samza/util/SinglePartitionWithoutOffsetsSystemAdmin.java > 01997ae22641b735cd452a0e89a49219e2874892 > samza-api/src/test/java/org/apache/samza/config/TestConfig.java > d9f378d8de6da3bdf002e69dfb1e4605c3d90cec > > samza-api/src/test/java/org/apache/samza/system/TestSystemStreamPartitionIterator.java > 5af2a11812983cfb8b6a8a0927ef6f3eba7d340f > samza-api/src/test/java/org/apache/samza/util/TestBlockingEnvelopeMap.java > 4eb87eb9033a45b5367480b9e38e18c629c79bc0 > > samza-core/src/main/java/org/apache/samza/serializers/model/SamzaObjectMapper.java > 3517912eaafbf95f8c8cc70ab5869548a56b76e7 > > samza-core/src/main/scala/org/apache/samza/container/grouper/task/GroupByContainerCount.scala > 8071fec3ac1bad76ddb3c6116e35c646be71d891 > samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueIterator.java > 2fb26e28685928547342b325ff6f0a63b3d83887 > samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueStore.java > b708341abed15aaad34df5934f5f310bc1feb87a > > samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java > d3f25c0e03a727e64a774581384ef5aae9ef9c1c > > samza-log4j/src/main/java/org/apache/samza/logging/log4j/serializers/LoggingEventStringSerde.java > 8d8f5e8a8e5fd1e4d9e5482a5accd4a7ece463bc > > samza-log4j/src/test/java/org/apache/samza/config/TestLog4jSystemConfig.java > 6314a3ed7ae6d49328ba3f32af5d9d1097899009 > > samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestJmxAppender.java > 0bdade0d7c097bcb33acdfc8077c1ce57ad7988c > samza-test/src/main/java/org/apache/samza/system/mock/MockSystemAdmin.java > c0a20af5a2f4329ad4a2cff378ced3bececbc1cb > > samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java > 67e56e0dd7b5cd2b636699f253e38c6265abf677 > > samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java > d7fecd83abdb1a171140cb13e17661a06e1d77c3 > > samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java > 0598e51c158d2865ddd40685b9a3243561b4f556 > > samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java > 82a633d4e796cde347c4398d85e1903be735d067 > > samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java > 438d77c0d393727d3e6fde19a2706efd6ea1ce09 > samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java > d2c0c7eaf9c389e3f88b63a2eb7668b31d1b2daf > > samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java > 7c82e0a7ac6b175cca935fc058a96aaade92fbe0 > > Diff: https://reviews.apache.org/r/33488/diff/ > > > Testing > ------- > > ./gradlew checkstyleMain checkstyleTest > > > Thanks, > > Guozhang Wang > >
