[ 
https://issues.apache.org/jira/browse/CASSANDRA-4383?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13447385#comment-13447385
 ] 

Eric Evans commented on CASSANDRA-4383:
---------------------------------------

bq. That's basically syntactic sugar over 
Gossiper.addLocalApplicationStates(...) which we'd still need as a building 
block. I've given up on my previous attempt which actually did not provide 
atomicity, and it looks like doing that would require rewriting EndpointState 
to use the Reference and SnapTree approach similar to AtomicSortedColumns, and 
that is way, way out of scope for this ticket (especially since we don't 
actually need it)

I was more or less looking for someway of safeguarding against future 
reordering of those two ops.  A comment warning future patch-writers would 
probably work too.

And on that note, in {{StorageService.bootstrap(...)}}, shouldn't {{TOKENS}} 
come first?

One other minor nit, {{VersionedValue.tokens(...)}} should use 
{{TokenSerializer.serialize(...)}} statically.

bq. I fixed [the harmless bootstrap NPE] in this patch too, by simple 
confirming that TOKENS is actually present for the host in 
handleStateBootstrap, and if not, just handle things the old way since it's a 
legacy-style bootstrap.

Probably out of scope since this patch never changed anything in this regard, 
but I wonder if {{StorageService.handleState\{Bootstrap,Normal\}(...)}} 
wouldn't benefit from some logging when they encounter a non-host ID enabled 
endpoint.  It's a rare but noteworthy event.  If you don't want to slip that 
change in pre-commit, let me know and I might open another ticket for that.

Aside from this, LGTM +1
                
> Binary encoding of vnode tokens
> -------------------------------
>
>                 Key: CASSANDRA-4383
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-4383
>             Project: Cassandra
>          Issue Type: Sub-task
>            Reporter: Brandon Williams
>            Assignee: Brandon Williams
>             Fix For: 1.2.0 beta 1
>
>         Attachments: 
> 0001-Add-HOST_ID-and-TOKENS-app-states-binary-serialization.txt, 
> 0002-Fix-tests.txt
>
>
> Since after CASSANDRA-4317 we can know which version a remote node is using 
> (that is, whether it is vnode-aware or not) this a good opportunity to change 
> the token encoding to binary, since with a default of 256 tokens per node 
> even a fixed-length 16 byte encoding per token provides a great deal of 
> savings in gossip traffic over a text representation.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to