> On Feb. 4, 2015, 2:15 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/OffsetCommitRequest.scala, line 48
> > <https://reviews.apache.org/r/27391/diff/11/?file=832423#file832423line48>
> >
> > I our convention is to include the if in the previous line.
>
> Guozhang Wang wrote:
> I checked the code base and it seems we do not have a consensus here..
> and personally I would prefer this as it actually make the logic clearer.
We don't have a formal convention here but I think we should and incorporate it
into our coding guidelines. The problem with a separate line is that at first
glance (especially with just two character indentation) it does not seem to be
associated with the assignment. Also, most current occurrences put the if on
the same line.
```
find . -name "*.scala" -exec pcregrep -c '=(\s)*if' {} \; | grep -v 0 | paste
-s -d+ | bc
61
find . -name "*.scala" -exec pcregrep -Mc '=(\s)*\n(\s)*if' {} \; | grep -v 0 |
paste -s -d+ | bc
36
```
> On Feb. 4, 2015, 2:15 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/common/OffsetMetadataAndError.scala, line 36
> > <https://reviews.apache.org/r/27391/diff/11/?file=832425#file832425line36>
> >
> > (This is also a public API change - although you did add an Object
> > wrapper further down that comes close to the original API.)
>
> Guozhang Wang wrote:
> I think the wrapper MessageAndMetadata preserves the existing public API
> right?
You mean the wrapper object? It comes close, but not quite - since you can
instantiate a case class with a `new` keyword or without. You need it for the
secondary constructors of the case class. With the object wrapper we assume
that the objects were being constructed without the new. I don't know how many
people actually used it though, but it was part of the public API since you
would need to create those objects to form an OffsetCommitRequest.
> On Feb. 4, 2015, 2:15 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 163
> > <https://reviews.apache.org/r/27391/diff/11/?file=832427#file832427line163>
> >
> > Shouldn't the commit timestamp _always_ be set to the current time?
> >
> > What I was thinking is this:
> > If v0:
> > - An explicit timestamp is provided only to override the v0 default
> > retention which is add the server-side retention to the current timestamp.
> > The (true) commit timestamp - i.e., receive time is useful for debugging
> > purposes. So if an explicit timestamp is provided in v0 then use that to
> > compute the absolute expire timestamp which will be the given commit
> > timestamp; so you would store (commitTimestamp = now; expireTimestamp =
> > given commitTimeStamp); if v0 and commit timestamp is default, then you
> > would store (commitTimestamp = now, expireTimestamp = now + offsetRetention)
> > - if v1: (commitTimestamp = now, expireTimestamp = now +
> > offsetRetention)
> >
> > This way, you should have correct expiration behavior for v0, v1 and v2
> > and at the same time have the true commit timestamp - i.e., the receive
> > time at the broker which is useful for debugging. (also see comment in
> > OffsetManager)
>
> Guozhang Wang wrote:
> In v0/v1, the commit timestamp can be specified as a future timestamp so
> the expiration timestamp = commit timestamp + retention (in v0/v1 it is
> always the default value).
>
> This behavior should not be respected, i.e. offsets already stored in v0
> and v1 format should be expired correctly using 0.8.2 code. Details can be
> found in Jun's comments and my replies.
I don't think we are on the same page here. Let's discuss offline to follow-up.
> On Feb. 4, 2015, 2:15 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 65
> > <https://reviews.apache.org/r/27391/diff/11/?file=832430#file832430line65>
> >
> > Should we call this maxOffsetRetentionMs instead?
>
> Guozhang Wang wrote:
> Not exactly, as it is just the default offset retention, not the upper
> limit: users can specify a value larger than this default and it will still
> be accepted.
Yes you are right.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27391/#review70836
-----------------------------------------------------------
On Feb. 6, 2015, 7:01 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27391/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2015, 7:01 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1634
> https://issues.apache.org/jira/browse/KAFKA-1634
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Joel's comments
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
> 09a6f11163ecb1e733c604ade04646e83bbc0c85
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java
> 101f382170ad6740b3f8ff2d27b93a64874a857f
> clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java
> ff89f0e37d5fa787b0218eff86d169aaeae2107b
>
> clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
> 94e9d376235b3288836807d8e8d2547b3743aad5
>
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
> 13237fd72da5448a3d596b882fef141f336f827d
> core/src/main/scala/kafka/api/OffsetCommitRequest.scala
> 050615c72efe7dbaa4634f53943bd73273d20ffb
> core/src/main/scala/kafka/api/OffsetFetchRequest.scala
> c7604b9cdeb8f46507f04ed7d35fcb3d5f150713
> core/src/main/scala/kafka/common/OffsetMetadataAndError.scala
> 4cabffeacea09a49913505db19a96a55d58c0909
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
> 5487259751ebe19f137948249aa1fd2637d2deb4
> core/src/main/scala/kafka/server/KafkaApis.scala
> f2b027bf944e735fd52cc282690ec1b8395f9290
> core/src/main/scala/kafka/server/KafkaConfig.scala
> 6d74983472249eac808d361344c58cc2858ec971
> core/src/main/scala/kafka/server/KafkaServer.scala
> 89200da30a04943f0b9befe84ab17e62b747c8c4
> core/src/main/scala/kafka/server/OffsetManager.scala
> 0bdd42fea931cddd072c0fff765b10526db6840a
> core/src/main/scala/kafka/server/ReplicaManager.scala
> fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala
> a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala
> 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5
> core/src/test/scala/unit/kafka/server/ServerShutdownTest.scala
> ba1e48e4300c9fb32e36e7266cb05294f2a481e5
>
> Diff: https://reviews.apache.org/r/27391/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
