Transition to BSER v2. #270

dhruvsinghal · 2016-06-09T18:22:01Z

As per https://github.com/facebook/watchman/wiki/Better-Unicode-handling-plan.

Added tests specific to BSER v1 (to make sure things don't break during the transition).
Renamed STRING to BYTESTRING.
Added context type for switching BSER versions.
Added BSER version switching logic to server.
Added BSER v2 as a capability testable with the version command.
Added logic for negotiating up to BSER v2 if both client and server support it.

ghost · 2016-06-09T19:15:43Z

@dhruvsinghal updated the pull request.

sunshowers · 2016-06-09T19:29:37Z

bser.c

@@ -48,7 +48,7 @@ static int bser_real(double val, json_dump_callback_t dump, void *data)
  return dump((char*)&val, sizeof(val), data);
 }

-bool bunser_string(const char *buf, json_int_t avail, json_int_t *needed,
+bool bunser_bytestring(const char *buf, json_int_t avail, json_int_t *needed,


(This should actually be a static function. Feel free to change it to one.)

sunshowers · 2016-06-09T19:33:55Z

So something to think about is making sure we have the testing story straight for old server/new client and new server/old client. It's probably not something that you'll have to deal with right now but it'll become important very soon.

ghost · 2016-06-09T20:35:54Z

@dhruvsinghal updated the pull request.

facebook-github-bot · 2016-06-09T22:28:25Z

@dhruvsinghal updated the pull request.

ghost · 2016-06-10T23:11:36Z

@dhruvsinghal updated the pull request.

ghost · 2016-06-11T00:50:42Z

@dhruvsinghal updated the pull request.

ghost · 2016-06-11T00:50:54Z

@dhruvsinghal updated the pull request.

wez · 2016-06-11T04:09:40Z

I took a brief look over this, and it is looking good!

The CI failures appear to be due to unconditionally emitting the new UTF8 string encoding; I think you'll need to add a way to explicitly enable that while defaulting to disabled to make them pass and then I think you're about ready to land this initial series--so long as it doesn't change or break the default behavior it's ok to land partially done work towards the larger goal.

wez · 2016-06-11T04:13:32Z

bser.c

@@ -48,7 +50,8 @@ static int bser_real(double val, json_dump_callback_t dump, void *data)
  return dump((char*)&val, sizeof(val), data);
 }

-bool bunser_string(const char *buf, json_int_t avail, json_int_t *needed,
+/* Common codepath for bytestring and UTF-8 encoded strings. */
+static bool bunser_string(const char *buf, json_int_t avail, json_int_t *needed,


you may want to rename this or change the type signature to help ensure that you got all references to this name and replaced them with bunser_bytestring

This is actually fine -- in d443952 @dhruvsinghal renamed bunser_string to bunser_bytestring.

ghost · 2016-06-13T19:45:44Z

@dhruvsinghal updated the pull request.

ghost · 2016-06-14T00:53:21Z

@dhruvsinghal updated the pull request.

dhruvsinghal · 2016-06-14T00:54:17Z

@Sid0 Can you have a look sometime and say if the refactor involving the context variable is on the right track? (Note that this build won't pass the tests yet since ASAN found bugs)

sunshowers · 2016-06-14T18:22:26Z

bser.c

 {
  json_int_t m_size = 0;
+  bser_ctx_t measurement_ctx = *ctx;
+  ctx->dump = measure;


measurement_ctx->dump = measure.

ghost · 2016-07-22T22:44:42Z

@dhruvsinghal updated the pull request.

ghost · 2016-07-25T21:40:03Z

@dhruvsinghal updated the pull request.

ghost · 2016-08-15T18:02:33Z

@dhruvsinghal updated the pull request.

sunshowers · 2016-08-15T18:15:59Z

bser.c

@@ -353,7 +372,16 @@ int w_bser_dump(const bser_ctx_t* ctx, json_t *json, void *data)
    case JSON_INTEGER:
      return bser_int(ctx, json_integer_value(json), data);
    case JSON_STRING:
-      return bser_bytestring(ctx, json_string_value(json), data);
+      wstr = json_to_w_string(json);


Wouldn't this affect BSERv1 as well?

Ah yes. This is a WIP, got pushed prematurely, when I rebased on the latest head and pushed.

facebook-github-bot · 2016-08-17T22:16:50Z

@dhruvsinghal updated the pull request - view changes

facebook-github-bot · 2016-08-18T01:47:19Z

@dhruvsinghal updated the pull request - view changes

…lback logic once BSER v2 becomes default.

side).

… client side).

… fixed.

facebook-github-bot · 2016-08-18T20:43:05Z

@dhruvsinghal updated the pull request - view changes

ghost · 2016-08-18T21:33:00Z

@dhruvsinghal updated the pull request - view changes

ghost · 2016-08-18T21:35:57Z

@dhruvsinghal updated the pull request - view changes

sunshowers · 2016-08-19T00:26:30Z

bser.c

@@ -401,7 +401,7 @@ int w_bser_write_pdu(const uint32_t bser_version,
  }

  if (bser_version == 2) {
-    if (bser_int(&ctx, bser_capabilities, data)) {
+    if (dump((const char*) &bser_capabilities, 4, data)) {


Could you make the 4 a #define or similar?

Made it sizeof(bser_capabilities)

wez · 2016-09-16T20:42:03Z

I'm closing this out because it has been idle for a while and has conflicts. Please open a new pull request rebased off master when you're ready to follow up on this work.

ghost added GH Review: review-needed CLA Signed labels Jun 9, 2016

sunshowers reviewed Jun 9, 2016
View reviewed changes

dhruvsinghal force-pushed the python3 branch from adb9422 to d0a7a28 Compare June 9, 2016 22:28

dhruvsinghal force-pushed the python3 branch from d0a7a28 to 2a91ffb Compare June 10, 2016 23:11

dhruvsinghal force-pushed the python3 branch from 2a91ffb to aebddd1 Compare June 11, 2016 00:47

wez reviewed Jun 11, 2016
View reviewed changes

dhruvsinghal force-pushed the python3 branch from aebddd1 to fdbb2c7 Compare June 13, 2016 19:45

dhruvsinghal force-pushed the python3 branch from fdbb2c7 to da20a68 Compare June 14, 2016 00:52

sunshowers reviewed Jun 14, 2016
View reviewed changes

bser.c

{

json_int_t m_size = 0;

bser_ctx_t measurement_ctx = *ctx;

ctx->dump = measure;

Copy link

Contributor

sunshowers Jun 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

measurement_ctx->dump = measure.

dhruvsinghal force-pushed the python3 branch 2 times, most recently from fdbb2c7 to 21ffd60 Compare June 14, 2016 18:29

dhruvsinghal force-pushed the python3 branch from 90ead54 to 62b417a Compare July 25, 2016 21:39

dhruvsinghal force-pushed the python3 branch from 62b417a to 534675f Compare August 15, 2016 18:02

sunshowers reviewed Aug 15, 2016
View reviewed changes

dhruvsinghal force-pushed the python3 branch from 534675f to cf36c9c Compare August 17, 2016 22:16

dhruvsinghal force-pushed the python3 branch from cf36c9c to 0ae8c1a Compare August 18, 2016 01:46

Dhruv Singhal added 8 commits August 18, 2016 13:42

Force the BserV2WithFallback codec to use BSER v2. Reactivate the fal…

8d5327e

…lback logic once BSER v2 becomes default.

Changed BSER capabilities bitvector to be fixed length (on the server

fb4c3d9

side).

Changed BSER capabilities bitvector to be fixed length (on the python…

9940fc0

… client side).

Enabled emission of unicode strings from the server side.

9996a3e

Enabled BSER v2 on python client side.

f749346

Adds bser-v2 as a capability.

cf2947d

Disabled UTF-8 emission in BSER v2 via capability until the tests are…

3a6eb42

… fixed.

Added escaping for mixed string type.

0577568

dhruvsinghal force-pushed the python3 branch from 0ae8c1a to 0577568 Compare August 18, 2016 20:42

Test case fixes for BSER v2.

a475459

Reenabled fallback for BSER v2 codec.

823179f

sunshowers reviewed Aug 19, 2016
View reviewed changes

wez closed this Sep 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transition to BSER v2. #270

Transition to BSER v2. #270

dhruvsinghal commented Jun 9, 2016 •

edited

ghost commented Jun 9, 2016

sunshowers Jun 9, 2016

sunshowers commented Jun 9, 2016

ghost commented Jun 9, 2016

facebook-github-bot commented Jun 9, 2016

ghost commented Jun 10, 2016

ghost commented Jun 11, 2016

ghost commented Jun 11, 2016

wez commented Jun 11, 2016

wez Jun 11, 2016

sunshowers Jun 13, 2016

ghost commented Jun 13, 2016

ghost commented Jun 14, 2016

dhruvsinghal commented Jun 14, 2016

sunshowers Jun 14, 2016

ghost commented Jul 22, 2016

ghost commented Jul 25, 2016

ghost commented Aug 15, 2016

sunshowers Aug 15, 2016

dhruvsinghal Aug 15, 2016

facebook-github-bot commented Aug 17, 2016

facebook-github-bot commented Aug 18, 2016

facebook-github-bot commented Aug 18, 2016

ghost commented Aug 18, 2016

ghost commented Aug 18, 2016

sunshowers Aug 19, 2016

dhruvsinghal Aug 19, 2016

wez commented Sep 16, 2016

Transition to BSER v2. #270

Transition to BSER v2. #270

Conversation

dhruvsinghal commented Jun 9, 2016 • edited

ghost commented Jun 9, 2016

Choose a reason for hiding this comment

sunshowers commented Jun 9, 2016

ghost commented Jun 9, 2016

facebook-github-bot commented Jun 9, 2016

ghost commented Jun 10, 2016

ghost commented Jun 11, 2016

ghost commented Jun 11, 2016

wez commented Jun 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 13, 2016

ghost commented Jun 14, 2016

dhruvsinghal commented Jun 14, 2016

Choose a reason for hiding this comment

ghost commented Jul 22, 2016

ghost commented Jul 25, 2016

ghost commented Aug 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 17, 2016

facebook-github-bot commented Aug 18, 2016

facebook-github-bot commented Aug 18, 2016

ghost commented Aug 18, 2016

ghost commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wez commented Sep 16, 2016

dhruvsinghal commented Jun 9, 2016 •

edited