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

googlebigtable2 - add binding for the current client api #1727

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

igorbernstein2
Copy link
Contributor

The existing googlebigtable binding uses an old client (bigtable-client-core) which is frozen and unmaintained. The new binding adds support for google-cloud-bigtable client

The existing googlebigtable binding uses an old client (bigtable-client-core) which is frozen and unmaintained. The new binding adds support for google-cloud-bigtable client
googlebigtable2/README.md Outdated Show resolved Hide resolved
googlebigtable2/README.md Outdated Show resolved Hide resolved
Optional<String> emulatorHost =
Optional.ofNullable(System.getenv().get("BIGTABLE_EMULATOR_HOST"));
Optional<String> dataEndpoint = Optional.ofNullable(props.getProperty(ENDPOINT_KEY));
if (emulatorHost.isPresent()) {
Copy link

Choose a reason for hiding this comment

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

Is this ever used? I don't think it's ever connected to the emulator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its used by the client and this is just to avoid conflicts between endpoint and BIGTABLE_EMULATOR_HOST values

.map(Boolean::parseBoolean)
.orElse(false);

Optional.ofNullable(props.getProperty(MAX_OUTSTANDING_BYTES))
Copy link

Choose a reason for hiding this comment

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

why making max outstadning bytes configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think patrick overrides this in internal tests

/**
* Tracks running thread counts so we know when to close the session.
*/
private static int clientRefCount = 0;
Copy link

Choose a reason for hiding this comment

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

should this be atomicInteger or volatile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its only ever updated in synchronized methods

mapToMutation(values, m);
getBatcher(table).add(m);

return Status.BATCHED_OK;
Copy link

Choose a reason for hiding this comment

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

If there are individual failures should we still return OK status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont think ycsb has any way to deal with batch entry failures :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants