Skip to content

Conversation

@mgodave
Copy link
Contributor

@mgodave mgodave commented Jan 29, 2018

Motivation

Allow users of the system to store the structure and format of the data in a topic

PIP: https://lists.apache.org/thread.html/6f5498bc90526efc44b0725eea0310b9332b4f71b04631233bd4daa1@%3Cdev.pulsar.apache.org%3E

Dave Rusek added 30 commits December 19, 2017 11:07
@mgodave mgodave mentioned this pull request Mar 14, 2018
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

overall looks good to me.

private CompletableFuture<LedgerHandle> openLedger(Long ledgerId) {
final CompletableFuture<LedgerHandle> future = new CompletableFuture<>();
bookKeeper.asyncOpenLedger(ledgerId, DigestType.MAC, new byte[]{},
bookKeeper.asyncOpenLedger(ledgerId, DigestType.MAC, "".getBytes(Charsets.UTF_8),
Copy link
Member

Choose a reason for hiding this comment

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

  • make "".getBytes(Charsets.UTF_8) a constant
  • use DigestType.CRC32C which is fastest than any other digest types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these can be taken from the service configuration.

import com.google.common.hash.Hashing;
import com.google.protobuf.ByteString;
import com.google.protobuf.InvalidProtocolBufferException;
import java.nio.ByteBuffer;
Copy link
Member

Choose a reason for hiding this comment

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

nit: the import seems not needed at all?

);

try {
// validateDestinationExists(destinationName);
Copy link
Member

Choose a reason for hiding this comment

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

remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

bin/pulsar Outdated
PULSAR_LOG_CONF=$DEFAULT_LOG_CONF
fi

OPTS="-agentlib:jdwp=transport=dt_socket,server=y,address=8000,suspend=n"
Copy link
Member

Choose a reason for hiding this comment

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

any idea why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I was debugging something and left this in.

private CompletableFuture<LedgerHandle> openLedger(Long ledgerId) {
final CompletableFuture<LedgerHandle> future = new CompletableFuture<>();
bookKeeper.asyncOpenLedger(ledgerId, DigestType.MAC, new byte[]{},
bookKeeper.asyncOpenLedger(ledgerId, DigestType.MAC, "".getBytes(Charsets.UTF_8),
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these can be taken from the service configuration.

private SchemaType type;
private long timestamp;
private String data;
private Map<String, String> props;
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for not using full word?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just (personal) convention. I'm not even consistent... Happy to change.

Dave Rusek added 8 commits March 20, 2018 09:28
- rename props to properties in GetSchemaResponse
- Use config for ledger parameters
# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/SchemasResource.java
#	pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java
#	pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java
@mgodave
Copy link
Contributor Author

mgodave commented Apr 9, 2018

All changes are merged, this tracking ticket can be closed or merged.

@sijie
Copy link
Member

sijie commented Apr 9, 2018

awesome work! @mgodave

@sijie sijie closed this Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature The PR added a new feature or issue requested a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants