Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: allow messages and producers to handle int64 sequences #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alanhoff
Copy link
Contributor

  • Allows the initial sequence ID to be passed as a string: client.createProducer({initialSequenceId: '1234'}) so JS can send 64 bits integers encoded as string.
  • Allows the message sequence ID to be passed as a string producer.send({sequenceId: '1234'}) so JS can send 64 bits integers encoded as string.
  • Implements Producer.getLastSequenceId which returns Napi::String since Napi::Number cannot be used for 64 bits integers.
  • Closes Allow BigInt/Buffer to be used as initialSequenceId and/or sequenceId #124

@alanhoff
Copy link
Contributor Author

alanhoff commented Oct 4, 2020

@hrsakai I've rebased my PR and it's ready for review 👍

@hrsakai hrsakai added this to the 1.3.0 milestone Oct 5, 2020
@hrsakai
Copy link
Contributor

hrsakai commented Oct 6, 2020

@alanhoff
I can't reproduce the issue.
I set 5294967295(greater than 32 bits integers) to initialSequenceId, but sequence id looks correct.
Please tell me how to reproduce.

initialSequenceId = 5294967295
lastSequenceId = 5294967395 

code

const Pulsar = require('../index.js');

(async () => {
  // Create a client
  const client = new Pulsar.Client({
    serviceUrl: 'pulsar://localhost:6650',
    operationTimeoutSeconds: 30,
  });

  // Create a producer
  const producer = await client.createProducer({
    topic: 'persistent://public/default/my-topic',
    sendTimeoutMs: 30000,
    batchingEnabled: false,
    initialSequenceId: 5294967295,
  });

  console.log('initialSequenceId = ' + producer.getLastSequenceId());
  // Send messages
  for (let i = 0; i < 100; i += 1) {
    const msg = `my-message-${i}`;
    await producer.send({
      data: Buffer.from(msg),
    });
  }
  console.log('lastSequenceId = ' + producer.getLastSequenceId());

  await producer.close();
  await client.close();
})();

@alanhoff
Copy link
Contributor Author

@hrsakai @sijie sorry for the delay here, let's start by correcting some of my misconceptions:

JS do support ints higher than 32 bits when representing them as a Number, but it may start losing precision for ints above 253 - 1 (or lower than the negative equivalent). Example:

const x = Number.MAX_SAFE_INTEGER + 1;
const y = Number.MAX_SAFE_INTEGER + 2;

console.log(x === y); // will output true

That said, we're "kinda" safe at the moment since it's the native addon that's incrementing the sequence using a proper int64_t, however IMHO I don't think the JS client should be using the Number primitive to represent an int64_t since that's unsafe.

The correct thing to do would be changing all sequences so they accept a BigInt instead of a Number, but it would be a breaking change and it wouldn't support Node.js < 10.4.0. So in the meanwhile this PR allows devs to optionally pass sequences as strings which are being casted into int64_t by the addon without losing precision.

@hrsakai
Copy link
Contributor

hrsakai commented Jan 14, 2021

@alanhoff
Thank you for your explanation.

Could you change initialSequenceId to accept NUMBER and BigInt and getLastSequenceId() to return BigInt?
We can accept that we don't support Node.js < 10.4.0.

@massakam massakam modified the milestones: 1.3.0, 1.4.0 Mar 8, 2021
@hrsakai hrsakai removed this from the 1.4.0 milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow BigInt/Buffer to be used as initialSequenceId and/or sequenceId
3 participants