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.

Sign up for GitHub

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

Jump to bottom

CASSSIDECAR-211 Config APIs for storing CDC/Kafka configs for CDC feature #193

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

CASSSIDECAR-211 Config APIs for storing CDC/Kafka configs for CDC feature #193

wants to merge 3 commits into from
+2,212 −10

Conversation

jyothsnakonisa
Copy link
Contributor

No description provided.

@jyothsnakonisa jyothsnakonisa force-pushed the cdc-config branch 3 times, most recently from 82a6c78 to 59eeaaa Compare February 13, 2025 23:06
bbotella
bbotella reviewed Feb 13, 2025
View reviewed changes
client/src/testFixtures/java/org/apache/cassandra/sidecar/client/SidecarClientTest.java
ObjectMapper mapper = new ObjectMapper();
response.setBody(mapper.writeValueAsString(putResponse));
enqueue(response);
assertThat(client.putCdcServiceConfig(ServiceConfig.CDC, payload).get()).isEqualTo(putResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add a test with an invalid payload (or service) to check that we are getting 400s?

@jyothsnakonisa jyothsnakonisa force-pushed the cdc-config branch 2 times, most recently from 35cbf00 to a42736a Compare February 14, 2025 22:11
yifan-c
yifan-c requested changes Feb 20, 2025
View reviewed changes
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfig.java Outdated
Comment on lines 32 to 35
Logger LOGGER = LoggerFactory.getLogger(CdcConfig.class);

String DEFAULT_JOB_ID = "test-job";
int DEFAULT_MAX_WATERMARKER_SIZE = 400000;
Copy link
Contributor

Choose a reason for hiding this comment

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

It is generally not advised to define constants in an interface. Can you relocate them to the implementation, CdcConfigImpl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The constant is used in the interface for default value and relocation to implementation will now work in this case

server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfigImpl.java
Comment on lines +55 to +69
private static final String CDC_CONFIG_DC_KEY = "dc";
private static final String CDC_CONFIG_LOG_ONLY_KEY = "log_only";
private static final String CDC_CONFIG_PERSIST_STATE_KEY = "persist_state";
private static final String CDC_CONFIG_ENV_KEY = "env";
private static final String KAFKA_CONFIG_TOPIC_KEY = "topic";
private static final String KAFKA_FORMAT_TYPE_CONFIG_TOPIC_KEY = "topic_format_type";
private static final String CDC_ENABLED_KEY = "cdc_enabled";
private static final String KAFKA_CONFIG_JOB_ID_KEY = "jobId";
private static final String WATERMARK_WINDOW_KEY = "watermark_seconds";
private static final String MICROBATCH_DELAY_KEY = "microbatch_delay_millis";
private static final String CDC_CONFIG_MAX_COMMIT_LOGS_KEY = "max_commit_logs";
private static final String CDC_MAX_WATERMARKER_SIZE_KEY = "max_watermarker_size";
private static final String CDC_FAIL_KAFKA_ERRORS = "fail_kafka_errors";
private static final String CDC_FAIL_KAFKA_TOO_LARGE_ERRORS = "fail_kafka_too_large_errors";
private static final String CDC_PERSIST_DELAY_MILLIS = "persist_delay_millis";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: define enums for the keys? and use the name() of each enum. It is less verbose.

server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorImpl.java
}

@Override
public Optional<ServiceConfig> storeConfigIfNotExists(Map<String, String> config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not used anywhere. Is there a mistake?
If it is no longer needed, can you delete the method as well as the relevant fields in the schema class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in some classes that we will be adding in other patches after this patch. we can remove it for now and add it or else we can add it now. I don't have a strong preference

server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorFactory.java Outdated
Comment on lines 41 to 51
if (service.equals(ValidServices.KAFKA.serviceName))
{
return kafkaConfigAccessor;
}

if (service.equals(ValidServices.CDC.serviceName))
{
return cdcConfigAccessor;
}

throw new RuntimeException("Couldn't find a db accessor for service " + service);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a map and do the look up?

configAccessors.get(service.toLowerCase()). If the result it null, it throws.

server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorFactory.java
Comment on lines +28 to +29
private final KafkaConfigAccessor kafkaConfigAccessor;
private final CdcConfigAccessor cdcConfigAccessor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove them. And have a map instead.

Map<String, ConfigAccessor> configAccessors

where the key is service name in lower case.

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 prefer having two separate objects, which are explicit instead of putting just two objects in the map and getting them with a string key.

@jyothsnakonisa jyothsnakonisa force-pushed the cdc-config branch 3 times, most recently from dd9e2eb to bc93d91 Compare February 20, 2025 21:40
frankgh
frankgh reviewed Feb 20, 2025
View reviewed changes
client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java
Comment on lines +127 to +128
public static final String SERVICE_CONFIG_ROUTE = API_V1 + SERVICES_PATH + SERVICE_PARAM + CONFIG;
public static final String GET_SERVICES_CONFIG_ROUTE = API_V1 + SERVICES_PATH;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the URI should be the same for the service, with different verbs to specify the action:

/api/v1/config/services/:service

  • GET : retrieve configuration for :service
  • DELETE: remove configuration for :service
  • PUT: update configuration for :service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The get verb gets config for all the services, I think it is valuable. If we want to be able to get configs of a given service we can add GET verb on /api/v1/config/services/:service.

@jyothsnakonisa jyothsnakonisa force-pushed the cdc-config branch 2 times, most recently from 9840126 to ff860db Compare February 20, 2025 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbotella bbotella bbotella left review comments

@jberragan jberragan jberragan left review comments

@frankgh frankgh frankgh left review comments

@yifan-c yifan-c yifan-c requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jyothsnakonisa @bbotella @jberragan @frankgh @yifan-c