You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/02/06 23:29:05 UTC

[kudu-CR] WIP: Add a new TIMESTAMP type

Adar Dembo has posted comments on this change.

Change subject: WIP: Add a new TIMESTAMP type
......................................................................


Patch Set 3:

(1 comment)

Just passing through...

http://gerrit.cloudera.org:8080/#/c/5819/3/src/kudu/util/timestamp_value.h
File src/kudu/util/timestamp_value.h:

Since this is client facing, here are a few other things to keep in mind:
1. Consider _not_ using inline functions, which place additional restrictions on the functions/fields they call.
2. Since it's not possible to add new fields once this has shipped, consider adding an extra int32_t or something of unused data as 'reserved', to be used in the future (for bit flags or something like that).

For more information on binary compatibility dos and donts, check out: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B


-- 
To view, visit http://gerrit.cloudera.org:8080/5819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I63271adf6b87f048e30bf2d6d04c758884752afc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes