samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Guozhang Wang" <wangg...@gmail.com>
Subject Re: Review Request 33488: SAMZA-657
Date Mon, 27 Apr 2015 20:00:00 GMT

-----------------------------------------------------------
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 (updated)
-----

  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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message