Features/protocol version flags #1090

mpilman · 2019-01-25T23:21:36Z

In order to make the code review process for flatbuffers less painful (as this will be a quite large change), I am going to make a code review early in the process so that people can take a look. The main ideas here are:

There are now object serializers that use flatbuffers to serialize and deserialize data. They are called object serializers as they do not support streaming (one can only serialize a root object). This serializer does not need the versioning and is fully upwards and downwards compatible if used correctly.
The version number includes a flag that will be set whenever a connecting process sends data that was serialized using this object serializer.
The idea will be to use a flag to instruct processes whether they should send data using this serializer. They can read objects serialized with either serializer (and decide which one to use by looking at the version number flag). This allows us to set this flag one process at a time. The reason this is done this way is because processes don't exchange versions both ways. The network code for this is mostly complete (though untested).
All data that is written to disk (for example to the system key-space) will still use the current serializer (which supports streaming and is order-preserving if it is unversioned). For newly introduced data, the user can chose which one to use.

atn34 · 2019-04-14T05:38:01Z

flow/Platform.cpp

 	if (!AssignProcessToJobObject( job, GetCurrentProcess() ))
 		TraceEvent(SevWarn, "FailedToSetMemoryLimit").GetLastError();
-#elif defined(__linux__)
+#elif defined(__linux__) && !defined(USE_ASAN)


This is no longer necessary as of eacde47

atn34 · 2019-04-15T16:27:44Z

flow/serialize.h

-	virtual void serializeBinaryWriter( BinaryWriter& ) const = 0;
+	virtual void serializePacketWriter(PacketWriter&, bool useObjectSerializer) const = 0;
+	virtual void serializeBinaryWriter(BinaryWriter&) const = 0;
+	virtual void serializeBinaryWriter(ObjectWriter&) const = 0;


serializeObjectWriter?

flow/serialize.h

xumengpanda · 2019-04-15T20:56:35Z

Need to test on Windows, but otherwise this is ready.

mpilman · 2019-04-17T00:47:50Z

This is now compiling on Windows as well. I tested:

MacOS X 10.14 with Apple LLVM version 10.0.1 (clang-1001.0.46.3)
Linux with gcc 8.2.1
Windows with Visual Studio 2017

atn34

Might be a good idea to clang-format just the hunks changed in this PR with git clang-format if you haven't already

atn34 · 2019-04-17T16:15:18Z

fdbclient/ClusterInterface.h

 	void serialize(Ar& ar) {
-		ASSERT( ar.protocolVersion() >= 0x0FDB00A400040001LL );
-		serializer(ar, issues, supportedVersions, connectedCoordinatorsNum, traceLogGroup, knownClientInfoID, reply, arena);
+		if constexpr (!is_fb_function<Ar>) {


Strange whitespace

atn34 · 2019-04-17T16:22:25Z

fdbmonitor/SimpleIni.h


        /** Strict less ordering by name of key only */
-        struct KeyOrder : std::binary_function<Entry, Entry, bool> {
+        struct KeyOrder {


This file is one of those third-party-don't-edit-it files. Please revert these changes (I'm assuming whether or not it's a good idea to include files we shouldn't edit in source control is out of scope for this PR)

If I revert this it will break the Windows build. std::binary_function was removed in C++17. So either we leave the change in or upgrade this external dependency. Fo now I will leave this in - updating SimpleIni.h should be done in a different PR.

atn34 · 2019-04-17T16:26:22Z

fdbrpc/ReplicationPolicy.h

-	    virtual bool validate(
-			std::vector<LocalityEntry>	const&	solutionSet,
-			Reference<LocalitySet> const&								fromServers ) const = 0;
+	IReplicationPolicy() {}


If I'm reading this right these are all whitespace-only changes? Might be good to move this to a separate PR

I can't work with this file without reformat. This file is so bad that I refuse to revert.

flow/FastRef.h

mpilman · 2019-04-17T17:44:09Z

Might be a good idea to clang-format just the hunks changed in this PR with git clang-format if you haven't already

the clang-format file in the repository is currently very far off what the general code-style is and it mixes spaces and tabs. So I kind of gave up in that fight...

AlvinMooreSr · 2019-05-09T17:50:02Z

test this please

mpilman · 2019-05-13T16:27:33Z

I don't quite understand why this is failing on MacOS. I tested this on MacOS and it works fine. If your version of Xcode doesn't support C++17 I would expect a failure earlier because of an unknown -std value...

This commit includes: - The flatbuffers implementation - A draft on how it should be used for network messages - A serializer that can be used independently What is missing: - All root objects will need a file identifier - Many special classes can not be serialized yet as the corresponding traits are not yet implemented - Object serialization can not yet be turned on (this will need a network option)

This commit includes functionality to turn on the object serializer for network communication. This is done the following way: - On incoming connections, a process will detect whether the client supports the object serializer and will only serialize responses with it, if it does - On outgoing connections, the command line flag is used to determine whether the object serializer should be used to send data. This way, a cluster can run in mixed mode. To upgrade one can upgrade one process at a time and set the flag one process at a time. This is how this is tested on the simulator: - The command line flag can take three options: on, off, and random. - For off, the object serializer will never we used. - For on, the object serializer will be always used. - For random, the simulator will flip a coin for each process it starts up.

When a process joins a cluster it fetches the cluster interface. However, not the whole interface is exposed to the client. This mechanism relies on the fact that the serializer keeps the field ordering and doesn't verify the message before parsing it. To make this work, we provide a client type with one member (the ClusterInterface which is exposed to the client and the server). This client interface has the same FileIdentifier as the ClusterControllerFullInterface which has the same first member. This works because FlatBuffers allows for members to be missing.

Currently, enabling the ObjectSerializer results in failures. These are hard to debug. Therefore I added a unit test that reproduces the problem.

Co-Authored-By: mpilman <[email protected]>

…t building via make

Add Flatbuffer To Flow Project

etschannen self-assigned this Jan 28, 2019

mpilman force-pushed the features/protocol-version-flags branch from 9082c32 to c577217 Compare January 29, 2019 03:48

mpilman force-pushed the features/protocol-version-flags branch from 43922f2 to a017f12 Compare February 20, 2019 17:52

hgray1 assigned mpilman and unassigned etschannen Feb 25, 2019

mpilman force-pushed the features/protocol-version-flags branch 2 times, most recently from 98a670a to 1b673fc Compare April 13, 2019 17:06

atn34 reviewed Apr 14, 2019

View reviewed changes

atn34 reviewed Apr 15, 2019

View reviewed changes

xumengpanda changed the title ~~Features/protocol version flags (For code review only)~~ Features/protocol version flags Apr 15, 2019

xumengpanda requested a review from etschannen April 15, 2019 20:56

mpilman force-pushed the features/protocol-version-flags branch 2 times, most recently from a8fe16b to 8125bba Compare April 17, 2019 00:44

atn34 reviewed Apr 17, 2019

View reviewed changes

mpilman force-pushed the features/protocol-version-flags branch from ca2ecb5 to 45e0234 Compare May 10, 2019 20:14

sfc-gh-mpilman added 10 commits May 13, 2019 14:15

Added Object serializer (does not yet compile)

8bbd283

types implemented

ba83c45

removed unused knob

1d5572a

Implementation complete (not yet working)

6afce01

disable rlimit when ASAN is used

1df3d8f

renamed flatbuffers tests for consistency

b06fe39

Added a test to reproduce current failure

8df7818

Currently, enabling the ObjectSerializer results in failures. These are hard to debug. Therefore I added a unit test that reproduces the problem.

sfc-gh-mpilman and others added 23 commits May 13, 2019 14:15

Fixed compilation issues after rebase

642a968

Support vector of unions in flatbuffers

0a36761

Added more low-level tests and implemented enums

7affe73

use EnsureTable for enums of enums

47fdb78

make EnsureTable work with streaming serializer

f0d81c6

fix indentation

3bc5ee8

fix addFlag

43a7703

fixed compilation issues after rebase

69fa3d3

pass NDEBUG to sqlite to enable debug mode

9c02354

Several flatbuffers bug fixes

44db345

some more fixes

f5fa3a6

removed unnecessary assert and added test

186bff4

Fix memory bug

f6fbad5

Fixed Tag serialization

0674556

Fixed issues introduced during rebase

42385c2

remove mixed-mode support

20c3f7f

Started to work on Windows

0713e06

Compiling on clang again

96aaa31

fixed merge error

57912b3

Remove obsolete function

b4fa13c

Added TODO to optimize packet serialization

a92233a

Co-Authored-By: mpilman <[email protected]>

address reviews and make compile with -Wunused-variable

46e7a0c

implemented std::set serializer for flatbuffers

5a13915

mpilman force-pushed the features/protocol-version-flags branch from 45e0234 to 5a13915 Compare May 13, 2019 21:16

AlvinMooreSr and others added 2 commits May 13, 2019 14:22

Added flat_buffers source files to Windows project in order to suppor…

b601fb2

…t building via make

Merge pull request #4 from AlvinMooreSr/flatbuffer

0e76395

Add Flatbuffer To Flow Project

etschannen merged commit a9b03ec into apple:master May 13, 2019

alexmiller-apple mentioned this pull request May 14, 2019

Set close-on-exec on all opened file descriptors #1581

Merged

Features/protocol version flags #1090

Features/protocol version flags #1090

Uh oh!

Conversation

mpilman commented Jan 25, 2019 • edited by xumengpanda Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atn34 Apr 14, 2019

Choose a reason for hiding this comment

Uh oh!

atn34 Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xumengpanda commented Apr 15, 2019

Uh oh!

mpilman commented Apr 17, 2019

Uh oh!

atn34 left a comment

Choose a reason for hiding this comment

Uh oh!

atn34 Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

atn34 Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

mpilman Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

atn34 Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

mpilman Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpilman commented Apr 17, 2019

Uh oh!

AlvinMooreSr commented May 9, 2019

Uh oh!

mpilman commented May 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mpilman commented Jan 25, 2019 •

edited by xumengpanda

Loading