Skip to content

Conversation

@FabianMeiswinkel
Copy link
Collaborator

No description provided.

@nehrao1 nehrao1 changed the base branch from main to thinclientEndpointDiscovery February 14, 2025 16:11
if (serverCertVerificationDisabled) {
sslContextBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE); // disable cert verification
}
sslContextBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE); // disable cert verification
Copy link
Owner

Choose a reason for hiding this comment

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

Assuming this would be removed after we do another round of cleanup, correct?

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we should remove it now honestly - I think it's risky to leave this as a TODO

OperationType.Read, ResourceType.Document, path, requestHeaders, options);

request.useThinProxy = Configs.isThinClientEnabled() && request.useGatewayMode ? true : false;
request.useThinProxy = true;
Copy link
Owner

@nehrao1 nehrao1 Feb 14, 2025

Choose a reason for hiding this comment

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

NIT: Remove this I think right? Leaving comment here for future tracking, non-blocking

public void encode(final ByteBuf out, boolean forThinClient) {

final int expectedLength = RntbdRequestFrame.LENGTH + this.headers.computeLength();
final int effectivePayloadSize = this.payload != null && this.payload.length > 0 ? this.payload.length + 4 : 0;
Copy link
Owner

@nehrao1 nehrao1 Feb 14, 2025

Choose a reason for hiding this comment

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

Non-blocking: why +4? I see it's only being used for logging but just wondering where this number came from

Copy link
Owner

@nehrao1 nehrao1 left a comment

Choose a reason for hiding this comment

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

Thank you very much Fabian - not sure if always disabling cert verification would be a good idea and I am scared we would forget to fix this later, could you please comment on this? Otherwise LGTM.

@nehrao1 nehrao1 merged commit d8f916b into nehrao1:thinclientEndpointDiscovery Feb 16, 2025
8 checks passed
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.

2 participants