Cassandra 7544 rebased2 #184

jasobrown · 2018-01-02T14:04:32Z

No description provided.

…one.

… reason.

…tchMBean.

jasobrown

OK, first round of review complete. On the whole, this looks great. I want to do a second round on selected stuffs, so look for that in the next day.

jasobrown · 2018-01-02T16:05:58Z

src/java/org/apache/cassandra/locator/InetAddressAndPort.java

+    @Override
+    public int compareTo(InetAddressAndPort o)
+    {
+        int retval = ByteBuffer.wrap(address.getAddress()).compareTo(ByteBuffer.wrap(o.address.getAddress()));


I'm not sure if compareTo is on a hot path anywhere, but I think we can avoid allocating the two ByteBuffers by using FastByteOperations#compareUnsigned

Yeah it's pretty bad. getAddress() allocates the address byte array. Maybe I should have these store the address array so I don't have to go to address to get it.

Get HostAddress is the same thing.

I think InetAddress implementations for whatever reason choose to trade footprint for allocations.

For us it's fine to just store the allocation. We don't plan on storing a ton of these I think.

Well it's not for whatever reason. They are defensive copies. We should consider the correctness issues here of not having defensive copies.

jasobrown · 2018-01-08T22:47:32Z

src/java/org/apache/cassandra/db/LegacySystemKeyspaceMigrator.java

+ * Can't just add the additional columns because they are primary key columns and C* doesn't support changing
+ * key columns even if it's just clustering columns.
+ */
+public class LegacySystemKeyspaceMigrator


trivial nit: maybe rename to SystemKeyspaceMigrator40 to indicate this is for 3.0/3.x -> 4.0 only, and that we drop after 4.0. I guess it's 'legacy' that's a little vague to me.

jasobrown · 2018-01-08T23:17:21Z

src/java/org/apache/cassandra/db/LegacySystemKeyspaceMigrator.java

+        logger.info("Migrated {} rows from legacy {} to {}", transferred, legacyPeerEventsName, peerEventsName);
+    }
+
+    static void migrateLegacyTransferredRanges()


pretty naming nit: remove 'legacy' from method name as you didn't do that on the other methods.

jasobrown · 2018-01-08T23:25:30Z

src/java/org/apache/cassandra/db/LegacySystemKeyspaceMigrator.java

+        int transferred = 0;
+        for (UntypedResultSet.Row row : rows)
+        {
+            logger.debug("Transferring row {}", transferred);


trivial nit: all three migrate* methods have this same log line. can you add the table name or something more unique?

Do we really want to repeat the table name on every line? There is a row at the end that lists the count of rows transferred and the source and destination. I could repeat that without the count before the transfer starts.

jasobrown · 2018-01-08T23:31:51Z

src/java/org/apache/cassandra/db/SystemKeyspace.java

+                + "PRIMARY KEY ((id)))")
+                .partitioner(new LocalPartitioner(TimeUUIDType.instance))
+                .compaction(CompactionParams.scts(singletonMap("min_threshold", "2")))
+                .gcGraceSeconds(0)


Is this suppossed to be here? It's not on trunk

I haven't rebased onto trunk in a while. Aleksey landed af3fe39#diff-ce3f6856b405c96859d9a50d9977e0b9L115 which is when it was removed on trunk. I'll get it when I rebase. And probably a lot of other pain as well. Looks like it's going to be 100% conflicts.

jasobrown · 2018-01-10T21:04:02Z

src/java/org/apache/cassandra/streaming/DefaultConnectionFactory.java

@@ -19,6 +19,7 @@
 package org.apache.cassandra.streaming;

 import java.io.IOException;
+import java.net.Socket;


nit: unused imports

jasobrown · 2018-01-10T21:39:21Z

src/java/org/apache/cassandra/tools/LoaderOptions.java

-        Set<InetAddress> ignores = new HashSet<>();
+        Set<InetAddress> hostsArg = new HashSet<>();
+        Set<InetAddress> ignoresArg = new HashSet<>();
+        Set<InetSocketAddress> hosts = new HashSet<>();


Why use InetSocketAddress here instead of InetAddressWithPort?

The loader is a client not really part of the database and interacts with the Java driver using InetSocketAddress.

The rule is that an InetSocketAddress should never refer to the storage port. InetAddressAndPort might refer to the client port though in some of the internal code in the server though.

jasobrown · 2018-01-10T22:07:17Z

src/java/org/apache/cassandra/tools/nodetool/Ring.java

-        Collection<String> leavingNodes = probe.getLeavingNodes();
-        Collection<String> movingNodes = probe.getMovingNodes();
-        Map<String, String> loadMap = probe.getLoadMap();
+        Collection<String> liveNodes = probe.getLiveNodes(false);


I'm not thrilled with the two versions of printDc, but given the objects they depend on (like SetHostStat), refactoring that is probably too much for this patch.

I know it's ugly, but eventually we can just get rid of the old way. It's going to converge on what we want soon enough.

jasobrown · 2018-01-10T22:57:15Z

src/java/org/apache/cassandra/transport/Server.java

-        private final Set<InetAddress> endpointsPendingJoinedNotification = ConcurrentHashMap.newKeySet();
-
-
-        private static final InetAddress bindAll;


I"m not sure you should delete this bindAll` check. There is a jira for it, CASSANDRA-5227. Jira is down right now so I can't see what the rationale behind this is.

I looked up the JIRA and it's not necessary anymore. The comment says

// Note that after all nodes are running a version that includes CASSANDRA-5899, rpcAddress should // never be 0.0.0.0, so this can eventually be removed.

So we can never hit that path anyway. I think it was useful when introduced, but it's not anymore.

hmm, looks like I didn't read that comment. But yes, you are correct, so +1 here

jasobrown · 2018-01-10T23:19:37Z

test/unit/org/apache/cassandra/cql3/PreparedStatementsTest.java

@@ -531,7 +530,7 @@ private void testPrepareWithLWT(ProtocolVersion version) throws Throwable
    @Test
    public void testPrepareWithBatchLWT() throws Throwable
    {
-        testPrepareWithBatchLWT(ProtocolVersion.V4);
+//        testPrepareWithBatchLWT(ProtocolVersion.V4);


Should this be commented out?

No I think I was debugging and commented it out.

jasobrown · 2018-01-16T23:50:58Z

src/java/org/apache/cassandra/streaming/StreamResultFuture.java

+        InetAddress connecting = (addr instanceof InetSocketAddress ? ((InetSocketAddress) addr).getAddress() : from.address);
+        //Need to turn connecting into a InetAddressAndPort with the correct port. I think getting the port from "from"
+        //Will work since we don't actually have ports diverge across network interfaces
+        StreamSession session = coordinator.getOrCreateSessionById(from, sessionIndex, InetAddressAndPort.getByAddressOverrideDefaults(connecting, from.port));


well, because I didn't document this (at all), it's hard to know why the hell I have to check the addr coming from the channel. Shame on me.

In the case of unit tests, if you use the EmbeddedChannel, channel.remoteAddress() does not return an InetSocketAddress, but an EmbeddedSocketAddress. I think the best thing to do here is:

private void attachConnection(InetAddressAndPort from, int sessionIndex, Channel channel) { SocketAddress addr = channel.remoteAddress(); final InetAddressAndPort connecting; if (addr instanceof InetSocketAddress) { InetSocketAddress address = (InetSocketAddress)addr; connecting = InetAddressAndPort.getByAddressOverrideDefaults(address.getAddress(), address.getPort()); } else { // assumably the addr is an EmbeddedSocketAddress, and we only get that when running unit tests where channel is an instance of EmbeddedChannel. In that case, it's safe to simply use the "from" parameter. connecting = from; } StreamSession session = coordinator.getOrCreateSessionById(from, sessionIndex, connecting); }

This doesn't look right? Your using the ephemeral port from SocketChannel.remoteAddress()? That's not a useful port number for anything.

Yes are correct; I made a mistake. We should keep your code, but can you add my comment: In the case of unit tests, if you use the EmbeddedChannel, channel.remoteAddress() does not return an InetSocketAddress, but an EmbeddedSocketAddress. Hence why we need the type check here

jasobrown · 2018-01-17T00:19:56Z

conf/cassandra.yaml

@@ -960,6 +967,7 @@ server_encryption_options:
    # cipher_suites: [TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_RSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA]
    # require_client_auth: false
    # require_endpoint_verification: false
+    # outgoing_encrypted_port_source: yaml


This field is not referenced anywhere in the code (not in ServerEncrpytionOptions). Is this just incomplete?

It's out of date. I implemented this functionality, but then you implemented and merged competing functionality. I'll remove it.

jasobrown · 2018-01-17T21:12:12Z

src/java/org/apache/cassandra/gms/ApplicationState.java

-    X1,
-    X2,
+    INTERNAL_ADDRESS_AND_PORT, //Replacement for INTERNAL_IP with up to two ports
+    NATIVE_ADDRESS_AND_PORT, //Replacement for RPC_ADDRESS


It's unclear to me why you renamed RPC_ADDRESS to NATIVE_ADDRESS. That make it really easy to confuse "the addr/port to be used between peers" as opposed to "the addr/port open for client apps/drivers". Perhaps a better name is INTERNODE_ADDRESS_AND_PORT? (or INTERNAL_ or PEER_ or ...)

native address is what it is now because thrift RPC is gone. Now what I remember is I did this because I thought we stopped using rpc_address in the yaml and renamed it to native_address. And now I am very confused because that doesn't seem to be the case. I was trying to consistently use native_address instead of rpc_address now that thrift is gone.

Not sure what the optimal thing to do here is. I didn't do this out of the blue and I didn't really want to, but I felt guilty about continuing to call it RPC. It's a lot of code changes to stop using native because there are many places where I made it consistent.

ughh, so I was completely wrong wrt the RPC_ADDRESS/NATIVE_ADDRESS thing. i misread this line:

appStates.put(ApplicationState.RPC_ADDRESS, valueFactory.rpcaddress(FBUtilities.getBroadcastRpcAddress()));

as this:

appStates.put(ApplicationState.RPC_ADDRESS, valueFactory.rpcaddress(FBUtilities.getBroadcastAddress()));

(getBroadcastRpcAddress vs getBroadcastAddress) when we do the assignment in StorageService.

…dd some logging.

…s now.

…we always pay the overhead anyways usually multiple times.

… ids aren't the same size.

… the list was made bigger than necessary.

aweisberg · 2018-01-23T16:36:08Z

src/java/org/apache/cassandra/db/SystemKeyspace.java

+                + "mean_partition_size bigint,"
+                + "partitions_count bigint,"
+                + "PRIMARY KEY ((keyspace_name), table_name, range_start, range_end))")
+                .gcGraceSeconds(0)


This is also not supposed to be here anymore.

… schema work.

aweisberg · 2018-01-23T20:03:07Z

I can't close this, but we should close it in favor of #188 which is this rebased yet again.

jasobrown · 2018-04-02T11:52:54Z

closing as 7544 has been committed.

aweisberg added 10 commits November 29, 2017 12:29

checkpoint to roll back

f612c60

Fix last broken unit test

9a6a949

Fix cqlsh.py bug and rename python driver so it picks up the current …

0be7dc2

…one.

Update python driver to rebased version

1e017b1

Update circle.yml to run dtests.

aa24172

Update java driver

2045f9f

Fix serialization bug and enable test for the bug.

729bb53

Don't randomly change the types of system table columns.

aef0fdc

Ended up with python driver that didn't have the goods in it for some…

a3ea6db

… reason.

Don't modify the behavior of a public interface in DynamicEndpointSni…

c83925c

…tchMBean.

jasobrown commented Jan 11, 2018

View reviewed changes

jasobrown commented Jan 16, 2018

View reviewed changes

jasobrown commented Jan 17, 2018

View reviewed changes

aweisberg added 12 commits January 22, 2018 15:46

Rename LegacySystemKeyspaceMigrator to SystemKeyspaceMigrator40 and a…

2121182

…dd some logging.

For forceBlockingFlush wait on flushes concurrently if possible.e

f7e5be6

Gossiper cleanup for review.

6c151f7

Optimize InetAddressAndPort to reduce allocations.

1a71fdb

Deprecate ValueFactory.bootReplacing(InetAddress) since it is ambigou…

457d4c7

…s now.

Memoize InetAddressAndPort address bytes since in most runtime cases …

35fbf3e

…we always pay the overhead anyways usually multiple times.

ForwardToContainer should fail on construction if targets and message…

90996e8

… ids aren't the same size.

Don't nest reference to ParameterType.FAILURE_REASON.

8255312

Silently ignore unrecognized parameters and continue deserialization.

b3ea07a

Improve comments on parameter tuple handling and fix sizing bug where…

1c23817

… the list was made bigger than necessary.

Bring back commented out test.

2850f7b

Whitespace cleanup

549b6ac

aweisberg reviewed Jan 23, 2018

View reviewed changes

aweisberg added 2 commits January 23, 2018 11:37

Don't bring back gcgraceseconds that was removed as part of Aleksey's…

80f0eea

… schema work.

Remove unused config option.

621569b

jasobrown closed this Apr 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cassandra 7544 rebased2 #184

Cassandra 7544 rebased2 #184

jasobrown commented Jan 2, 2018

jasobrown left a comment

jasobrown Jan 2, 2018

aweisberg Jan 22, 2018

aweisberg Jan 22, 2018

aweisberg Jan 22, 2018 •

edited

jasobrown Jan 8, 2018

jasobrown Jan 8, 2018

jasobrown Jan 8, 2018

aweisberg Jan 22, 2018

jasobrown Jan 8, 2018

aweisberg Jan 22, 2018

jasobrown Jan 10, 2018

jasobrown Jan 10, 2018

aweisberg Jan 22, 2018

jasobrown Jan 10, 2018

aweisberg Jan 22, 2018

jasobrown Jan 23, 2018

jasobrown Jan 10, 2018

aweisberg Jan 22, 2018

jasobrown Jan 23, 2018

jasobrown Jan 10, 2018

aweisberg Jan 22, 2018

jasobrown Jan 16, 2018

aweisberg Jan 22, 2018

jasobrown Jan 23, 2018

jasobrown Jan 17, 2018

aweisberg Jan 23, 2018 •

edited

jasobrown Jan 17, 2018

aweisberg Jan 23, 2018

aweisberg Jan 23, 2018

jasobrown Jan 24, 2018

aweisberg Jan 23, 2018

aweisberg commented Jan 23, 2018

jasobrown commented Apr 2, 2018

		private final Set<InetAddress> endpointsPendingJoinedNotification = ConcurrentHashMap.newKeySet();


		private static final InetAddress bindAll;

Cassandra 7544 rebased2 #184

Cassandra 7544 rebased2 #184

Conversation

jasobrown commented Jan 2, 2018

jasobrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweisberg Jan 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweisberg Jan 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweisberg commented Jan 23, 2018

jasobrown commented Apr 2, 2018

aweisberg Jan 22, 2018 •

edited

aweisberg Jan 23, 2018 •

edited